Close WebXDC zip files explicitly

Amolith created

WebXDC preview and message rows were constructing page objects without a
lifecycle owner that released the backing ZipFile. Keep ZipFile
ownership inside active sessions, close previews after metadata and icon
reads, and make session removal close pages explicitly.

Avoid constructing long-lived WebxdcPage instances during message
binding. Use cached file metadata for the initial button label, refresh
the manifest title in the background, and load icons through the
existing file attachment executor.

Change summary

src/cheogram/java/com/cheogram/android/ConversationPage.java          |   1 
src/cheogram/java/com/cheogram/android/ExtensionSettingsFragment.java | 127 
src/cheogram/java/com/cheogram/android/WebxdcPage.java                |  43 
src/main/java/eu/siacs/conversations/entities/Conversation.java       |   6 
src/main/java/eu/siacs/conversations/persistance/FileBackend.java     |  15 
src/main/java/eu/siacs/conversations/ui/ConversationFragment.java     |  35 
src/main/java/eu/siacs/conversations/ui/adapter/MessageAdapter.java   |  63 
7 files changed, 204 insertions(+), 86 deletions(-)

Detailed changes

src/cheogram/java/com/cheogram/android/ExtensionSettingsFragment.java 🔗

@@ -41,6 +41,7 @@ import eu.siacs.conversations.worker.ExportBackupWorker;
 
 public class ExtensionSettingsFragment extends androidx.fragment.app.Fragment {
 	FragmentExtensionSettingsBinding binding;
+	ExtensionAdapter extensionAdapter;
 
 	@Override
 	public void onCreate(Bundle savedInstanceState) {
@@ -59,38 +60,9 @@ public class ExtensionSettingsFragment extends androidx.fragment.app.Fragment {
 			startActivityForResult(Intent.createChooser(intent, getString(R.string.perform_action_with)), 0x1);
 		});
 
-		binding.extensionList.setAdapter(new RecyclerView.Adapter<WebxdcViewHolder>() {
-			final ArrayList<WebxdcPage> xdcs = new ArrayList<>();
-
-			@Override
-			public int getItemCount() {
-				xdcs.clear();
-				final var activity = (XmppActivity) requireActivity();
-				final var xmppConnectionService = activity.xmppConnectionService;
-				if (xmppConnectionService == null) return xdcs.size();
-				final var dir = new File(xmppConnectionService.getExternalFilesDir(null), "extensions");
-				for (File file : Files.fileTraverser().breadthFirst(dir)) {
-					if (file.isFile() && file.canRead()) {
-						final var dummy = new Message(new StubConversation(null, "", null, 0), null, Message.ENCRYPTION_NONE);
-						dummy.setStatus(Message.STATUS_DUMMY);
-						dummy.setUuid(file.getName());
-						xdcs.add(new WebxdcPage(activity, file, dummy));
-					}
-				}
-				return xdcs.size();
-			}
-
-			@Override
-			public WebxdcViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
-				final ExtensionItemBinding binding = DataBindingUtil.inflate(inflater, R.layout.extension_item, container, false);
-				return new WebxdcViewHolder(binding);
-			}
-
-			@Override
-			public void onBindViewHolder(WebxdcViewHolder holder, int position) {
-				holder.bind(xdcs.get(position));
-			}
-		});
+		extensionAdapter = new ExtensionAdapter(inflater);
+		binding.extensionList.setAdapter(extensionAdapter);
+		extensionAdapter.refresh();
 
 		return binding.getRoot();
 	}
@@ -104,6 +76,17 @@ public class ExtensionSettingsFragment extends androidx.fragment.app.Fragment {
 	public void onStart() {
 		super.onStart();
 		getActivity().setTitle("Extensions");
+		if (extensionAdapter != null) {
+			extensionAdapter.refresh();
+			extensionAdapter.notifyDataSetChanged();
+		}
+	}
+
+	@Override
+	public void onDestroyView() {
+		super.onDestroyView();
+		extensionAdapter = null;
+		binding = null;
 	}
 
 	public void addExtension(Uri uri) {
@@ -126,7 +109,71 @@ public class ExtensionSettingsFragment extends androidx.fragment.app.Fragment {
 		for (final var attachment : Attachment.extractAttachments(requireActivity(), data, Attachment.Type.FILE)) {
 			if ("application/webxdc+zip".equals(attachment.getMime())) addExtension(attachment.getUri());
 		}
-		binding.extensionList.getAdapter().notifyDataSetChanged();
+		if (extensionAdapter != null) {
+			extensionAdapter.refresh();
+			extensionAdapter.notifyDataSetChanged();
+		}
+	}
+
+	protected class ExtensionAdapter extends RecyclerView.Adapter<WebxdcViewHolder> {
+		final LayoutInflater inflater;
+		final ArrayList<ExtensionPreview> xdcs = new ArrayList<>();
+
+		ExtensionAdapter(final LayoutInflater inflater) {
+			this.inflater = inflater;
+		}
+
+		public void refresh() {
+			xdcs.clear();
+			final var activity = (XmppActivity) requireActivity();
+			final var xmppConnectionService = activity.xmppConnectionService;
+			if (xmppConnectionService == null) return;
+			final var dir = new File(xmppConnectionService.getExternalFilesDir(null), "extensions");
+			for (File file : Files.fileTraverser().breadthFirst(dir)) {
+				if (file.isFile() && file.canRead()) {
+					final var xdc = new WebxdcPage(activity, file, createExtensionPreviewSource(file));
+					try {
+						xdcs.add(new ExtensionPreview(file, xdc.getName()));
+					} finally {
+						xdc.close();
+					}
+				}
+			}
+		}
+
+		@Override
+		public int getItemCount() {
+			return xdcs.size();
+		}
+
+		@Override
+		public WebxdcViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
+			final ExtensionItemBinding binding = DataBindingUtil.inflate(inflater, R.layout.extension_item, parent, false);
+			return new WebxdcViewHolder(binding);
+		}
+
+		@Override
+		public void onBindViewHolder(WebxdcViewHolder holder, int position) {
+			holder.bind((XmppActivity) requireActivity(), xdcs.get(position));
+		}
+	}
+
+	private static Message createExtensionPreviewSource(final File file) {
+		final var source = new Message(new StubConversation(null, "", null, 0), null, Message.ENCRYPTION_NONE);
+		source.setStatus(Message.STATUS_DUMMY);
+		source.setUuid(file.getName());
+		return source;
+	}
+
+	protected static class ExtensionPreview {
+		final File file;
+		final String name;
+		android.graphics.drawable.Drawable icon;
+
+		ExtensionPreview(final File file, final String name) {
+			this.file = file;
+			this.name = name;
+		}
 	}
 
 	protected static class WebxdcViewHolder extends RecyclerView.ViewHolder {
@@ -137,9 +184,17 @@ public class ExtensionSettingsFragment extends androidx.fragment.app.Fragment {
 			this.binding = binding;
 		}
 
-		public void bind(WebxdcPage xdc) {
-			binding.icon.setImageDrawable(xdc.getIcon());
-			binding.name.setText(xdc.getName());
+		public void bind(final XmppActivity activity, ExtensionPreview xdc) {
+			if (xdc.icon == null) {
+				final var page = new WebxdcPage(activity, xdc.file, createExtensionPreviewSource(xdc.file));
+				try {
+					xdc.icon = page.getIcon();
+				} finally {
+					page.close();
+				}
+			}
+			binding.name.setText(xdc.name);
+			binding.icon.setImageDrawable(xdc.icon);
 		}
 	}
 }

src/cheogram/java/com/cheogram/android/WebxdcPage.java 🔗

@@ -93,7 +93,9 @@ public class WebxdcPage implements ConversationPage {
 			if (f != null) zip = new ZipFile(f);
 			final ZipEntry manifestEntry = zip == null ? null : zip.getEntry("manifest.toml");
 			if (manifestEntry != null) {
-				manifest = Toml.parse(zip.getInputStream(manifestEntry));
+				try (final InputStream is = zip.getInputStream(manifestEntry)) {
+					manifest = Toml.parse(is);
+				}
 			}
 		} catch (final IOException e) {
 			Log.w(Config.LOGTAG, "WebxdcPage: " + e);
@@ -116,22 +118,27 @@ public class WebxdcPage implements ConversationPage {
 
 	public Drawable getIcon(int dp) {
 		if (android.os.Build.VERSION.SDK_INT < 28) return null;
-		if (zip == null) return null;
-		ZipEntry entry = zip.getEntry("icon.webp");
-		if (entry == null) entry = zip.getEntry("icon.png");
-		if (entry == null) entry = zip.getEntry("icon.jpg");
-		if (entry == null) return null;
+		final ZipFile localZip = zip;
+		if (localZip == null) return null;
 
 		try {
+			ZipEntry entry = localZip.getEntry("icon.webp");
+			if (entry == null) entry = localZip.getEntry("icon.png");
+			if (entry == null) entry = localZip.getEntry("icon.jpg");
+			if (entry == null) return null;
 			DisplayMetrics metrics = xmppConnectionService.getResources().getDisplayMetrics();
-			ImageDecoder.Source source = ImageDecoder.createSource(ByteBuffer.wrap(ByteStreams.toByteArray(zip.getInputStream(entry))));
+			final byte[] data;
+			try (final InputStream is = localZip.getInputStream(entry)) {
+				data = ByteStreams.toByteArray(is);
+			}
+			ImageDecoder.Source source = ImageDecoder.createSource(ByteBuffer.wrap(data));
 			return ImageDecoder.decodeDrawable(source, (decoder, info, src) -> {
 				int w = info.getSize().getWidth();
 				int h = info.getSize().getHeight();
 				Rect r = FileBackend.rectForSize(w, h, (int)(metrics.density * dp));
 				decoder.setTargetSize(r.width(), r.height());
 			});
-		} catch (final IOException e) {
+		} catch (final IOException | IllegalStateException e) {
 			Log.w(Config.LOGTAG, "WebxdcPage.getIcon: " + e);
 			return null;
 		}
@@ -142,6 +149,19 @@ public class WebxdcPage implements ConversationPage {
 		return title == null ? "Widget" : title;
 	}
 
+	@Override
+	public void close() {
+		final ZipFile localZip = zip;
+		if (localZip == null) return;
+		try {
+			localZip.close();
+		} catch (final IOException e) {
+			Log.w(Config.LOGTAG, "WebxdcPage.close: " + e);
+		} finally {
+			zip = null;
+		}
+	}
+
 	public String getTitle() {
 		String title = manifest == null ? null : manifest.getString("name");
 		if (lastUpdate != null && lastUpdate.getDocument() != null) {
@@ -176,7 +196,8 @@ public class WebxdcPage implements ConversationPage {
 		Log.i(Config.LOGTAG, "interceptRequest: " + rawUrl);
 		WebResourceResponse res = null;
 		try {
-			if (zip == null) {
+			final ZipFile localZip = zip;
+			if (localZip == null) {
 				throw new Exception("no zip found");
 			}
 			if (rawUrl == null) {
@@ -187,13 +208,13 @@ public class WebxdcPage implements ConversationPage {
 				InputStream targetStream = xmppConnectionService.getResources().openRawResource(R.raw.webxdc);
 				res = new WebResourceResponse("text/javascript", "UTF-8", targetStream);
 			} else {
-				ZipEntry entry = zip.getEntry(path.substring(1));
+				ZipEntry entry = localZip.getEntry(path.substring(1));
 				if (entry == null) {
 					throw new Exception("\"" + path + "\" not found");
 				}
 				String mimeType = MimeUtils.guessFromPath(path);
 				String encoding = mimeType.startsWith("text/") ? "UTF-8" : null;
-				res = new WebResourceResponse(mimeType, encoding, zip.getInputStream(entry));
+				res = new WebResourceResponse(mimeType, encoding, localZip.getInputStream(entry));
 			}
 		} catch (Exception e) {
 			e.printStackTrace();

src/main/java/eu/siacs/conversations/entities/Conversation.java 🔗

@@ -2057,9 +2057,11 @@ public class Conversation extends AbstractEntity
         }
 
         public void removeSession(ConversationPage session) {
-            sessions.remove(session);
+            if (sessions == null) return;
+            if (!sessions.remove(session)) return;
+            session.close();
             notifyDataSetChanged();
-            if (session instanceof WebxdcPage) mPager.get().setCurrentItem(0);
+            if (session instanceof WebxdcPage && mPager.get() != null) mPager.get().setCurrentItem(0);
         }
 
         public boolean switchToSession(final String node) {

src/main/java/eu/siacs/conversations/persistance/FileBackend.java 🔗

@@ -1881,14 +1881,15 @@ public class FileBackend {
                 fileParams.runtime = getMediaRuntime(file);
             }
             if ("application/webxdc+zip".equals(mime)) {
-                try {
-                    final var zip = new ZipFile(file);
-                    final ZipEntry manifestEntry = zip == null ? null : zip.getEntry("manifest.toml");
+                try (final var zip = new ZipFile(file)) {
+                    final ZipEntry manifestEntry = zip.getEntry("manifest.toml");
                     if (manifestEntry != null) {
-                        final var manifest = Toml.parse(zip.getInputStream(manifestEntry));
-                        if (manifest != null) {
-                            final var name = manifest.getString("name");
-                            if (name != null) fileParams.setName(name);
+                        try (final var is = zip.getInputStream(manifestEntry)) {
+                            final var manifest = Toml.parse(is);
+                            if (manifest != null) {
+                                final var name = manifest.getString("name");
+                                if (name != null) fileParams.setName(name);
+                            }
                         }
                     }
                 } catch (final IOException e2) { }

src/main/java/eu/siacs/conversations/ui/ConversationFragment.java 🔗

@@ -285,7 +285,7 @@ public class ConversationFragment extends XmppFragment
     private final PendingItem<String> pendingLastMessageUuid = new PendingItem<>();
     private final PendingItem<Message> pendingMessage = new PendingItem<>();
     public Uri mPendingEditorContent = null;
-    protected ArrayList<WebxdcPage> extensions = new ArrayList<>();
+    protected ArrayList<File> extensions = new ArrayList<>();
     protected MessageAdapter messageListAdapter;
     protected CommandAdapter commandAdapter;
     private MediaPreviewAdapter mediaPreviewAdapter;
@@ -1818,6 +1818,7 @@ public class ConversationFragment extends XmppFragment
     public void onDestroyView() {
         super.onDestroyView();
         Log.d(Config.LOGTAG, "ConversationFragment.onDestroyView()");
+        extensions.clear();
         messageListAdapter.setOnContactPictureClicked(null);
         messageListAdapter.setOnContactPictureLongClicked(null);
         messageListAdapter.setOnInlineImageLongClicked(null);
@@ -2020,14 +2021,14 @@ public class ConversationFragment extends XmppFragment
             final var dir = new File(xmppConnectionService.getExternalFilesDir(null), "extensions");
             for (File file : Files.fileTraverser().breadthFirst(dir)) {
                 if (file.isFile() && file.canRead()) {
-                    final var dummy = new Message(conversation, null, conversation.getNextEncryption());
-                    dummy.setStatus(Message.STATUS_DUMMY);
-                    dummy.setThread(conversation.getThread());
-                    dummy.setUuid(file.getName());
-                    final var xdc = new WebxdcPage(activity, file, dummy);
-                    extensions.add(xdc);
-                    final var item = menu.add(0x1, extensions.size() - 1, 0, xdc.getName());
-                    item.setIcon(xdc.getIcon(24));
+                    final var xdc = new WebxdcPage(activity, file, createExtensionSourceMessage(file));
+                    try {
+                        extensions.add(file);
+                        final var item = menu.add(0x1, extensions.size() - 1, 0, xdc.getName());
+                        item.setIcon(xdc.getIcon(24));
+                    } finally {
+                        xdc.close();
+                    }
                 }
             }
             ConversationMenuConfigurator.configureAttachmentMenu(conversation, menu, TextUtils.isEmpty(binding.textinput.getText()));
@@ -2378,7 +2379,8 @@ public class ConversationFragment extends XmppFragment
             return super.onOptionsItemSelected(item);
         }
         if (item.getGroupId() == 0x1) {
-            conversation.startWebxdc(extensions.get(item.getItemId()));
+            final File file = extensions.get(item.getItemId());
+            conversation.startWebxdc(new WebxdcPage(activity, file, createExtensionSourceMessage(file)));
             return true;
         }
         switch (item.getItemId()) {
@@ -2472,6 +2474,14 @@ public class ConversationFragment extends XmppFragment
         return super.onOptionsItemSelected(item);
     }
 
+    private Message createExtensionSourceMessage(final File file) {
+        final var source = new Message(conversation, null, conversation.getNextEncryption());
+        source.setStatus(Message.STATUS_DUMMY);
+        source.setThread(conversation.getThread());
+        source.setUuid(file.getName());
+        return source;
+    }
+
     public boolean onBackPressed() {
         boolean wasLocked = conversation.getLockThread();
         conversation.setLockThread(false);
@@ -3946,11 +3956,10 @@ public class ConversationFragment extends XmppFragment
             }
             if (message == null) return;
 
-            Cid webxdcCid = message.getFileParams().getCids().get(0);
-            WebxdcPage webxdc = new WebxdcPage(activity, webxdcCid, message);
             Conversation conversation = (Conversation) message.getConversation();
             if (!conversation.switchToSession("webxdc\0" + message.getUuid())) {
-                conversation.startWebxdc(webxdc);
+                Cid webxdcCid = message.getFileParams().getCids().get(0);
+                conversation.startWebxdc(new WebxdcPage(activity, webxdcCid, message));
             }
         }
         if (message != null) {

src/main/java/eu/siacs/conversations/ui/adapter/MessageAdapter.java 🔗

@@ -180,6 +180,7 @@ public class MessageAdapter extends ArrayAdapter<Message> {
     private BubbleDesign bubbleDesign = new BubbleDesign(false, false, false, true);
     private final boolean mForceNames;
     private final Map<String, WebxdcUpdate> lastWebxdcUpdate = new HashMap<>();
+    private final Map<String, String> webxdcNames = new HashMap<>();
     private String selectionUuid = null;
     private final AppSettings appSettings;
 
@@ -847,25 +848,15 @@ public class MessageAdapter extends ArrayAdapter<Message> {
 
     private void displayWebxdcMessage(BubbleMessageItemViewHolder viewHolder, final Message message, final BubbleColor bubbleColor) {
         Cid webxdcCid = message.getFileParams().getCids().get(0);
-        WebxdcPage webxdc = new WebxdcPage(activity, webxdcCid, message);
+        final String webxdcName = getWebxdcName(webxdcCid, message);
         displayTextMessage(viewHolder, message, bubbleColor);
         viewHolder.image().setVisibility(View.GONE);
         viewHolder.audioPlayer().setVisibility(View.GONE);
         viewHolder.downloadButton().setVisibility(View.VISIBLE);
         viewHolder.downloadButton().setIconResource(0);
-        viewHolder.downloadButton().setText("Open " + webxdc.getName());
-        viewHolder.downloadButton().setOnClickListener(v -> {
-            Conversation conversation = (Conversation) message.getConversation();
-            if (!conversation.switchToSession("webxdc\0" + message.getUuid())) {
-                conversation.startWebxdc(webxdc);
-            }
-        });
-        viewHolder.image().setOnClickListener(v -> {
-            Conversation conversation = (Conversation) message.getConversation();
-            if (!conversation.switchToSession("webxdc\0" + message.getUuid())) {
-                conversation.startWebxdc(webxdc);
-            }
-        });
+        viewHolder.downloadButton().setText("Open " + webxdcName);
+        viewHolder.downloadButton().setOnClickListener(v -> openWebxdcMessage(webxdcCid, message));
+        viewHolder.image().setOnClickListener(v -> openWebxdcMessage(webxdcCid, message));
 
         final WebxdcUpdate lastUpdate;
         synchronized(lastWebxdcUpdate) { lastUpdate = lastWebxdcUpdate.get(message.getUuid()); }
@@ -890,13 +881,19 @@ public class MessageAdapter extends ArrayAdapter<Message> {
         final LruCache<String, Drawable> cache = activity.xmppConnectionService.getDrawableCache();
         final Drawable d = cache.get("webxdc:icon:" + webxdcCid);
         if (d == null) {
-            new Thread(() -> {
-                Drawable icon = webxdc.getIcon();
+            XmppConnectionService.FILE_ATTACHMENT_EXECUTOR.execute(() -> {
+                final WebxdcPage webxdc = new WebxdcPage(activity, webxdcCid, message);
+                final Drawable icon;
+                try {
+                    icon = webxdc.getIcon();
+                } finally {
+                    webxdc.close();
+                }
                 if (icon != null) {
                     cache.put("webxdc:icon:" + webxdcCid, icon);
                     activity.xmppConnectionService.updateConversationUi();
                 }
-            }).start();
+            });
         } else {
             viewHolder.image().setVisibility(View.VISIBLE);
             viewHolder.image().setImageDrawable(d);
@@ -904,6 +901,38 @@ public class MessageAdapter extends ArrayAdapter<Message> {
         }
     }
 
+    private String getWebxdcName(final Cid webxdcCid, final Message message) {
+        final String key = message.getUuid() == null ? webxdcCid.toString() : message.getUuid();
+        final String fallbackName = message.getFileParams().getName();
+        final String fallback = fallbackName == null ? "Widget" : fallbackName;
+        synchronized (webxdcNames) {
+            final String cached = webxdcNames.get(key);
+            if (cached != null) return cached;
+            webxdcNames.put(key, fallback);
+        }
+        XmppConnectionService.FILE_ATTACHMENT_EXECUTOR.execute(() -> {
+            final WebxdcPage webxdc = new WebxdcPage(activity, webxdcCid, message);
+            final String name;
+            try {
+                name = webxdc.getName();
+            } finally {
+                webxdc.close();
+            }
+            synchronized (webxdcNames) {
+                webxdcNames.put(key, name);
+            }
+            activity.xmppConnectionService.updateConversationUi();
+        });
+        return fallback;
+    }
+
+    private void openWebxdcMessage(final Cid webxdcCid, final Message message) {
+        final Conversation conversation = (Conversation) message.getConversation();
+        if (!conversation.switchToSession("webxdc\0" + message.getUuid())) {
+            conversation.startWebxdc(new WebxdcPage(activity, webxdcCid, message));
+        }
+    }
+
     private void displayOpenableMessage(
             final BubbleMessageItemViewHolder viewHolder,
             final Message message,