The default bug view has changed. See this FAQ.

Tab screenshots can hose the gecko event loop

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

unspecified
Firefox 12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

Created attachment 591604 [details] [diff] [review]
patch
Attachment #591604 - Flags: review?(mark.finkle)
Comment on attachment 591604 [details] [diff] [review]
patch

I'm not a big fan of the actual impl, but I understand the reason we would need this. This pass is a "can I make myself like this better" review

>diff -r 5f6d25767d30 -r e6336e3e1e8c mobile/android/base/TabsTray.java

>+        GeckoAppShell.sendEventToGecko(new GeckoEvent("Tab:CancelScreenshots",""));

nit: "Tab:Screenshot:Cancel"

>diff -r 5f6d25767d30 -r e6336e3e1e8c mobile/android/chrome/content/browser.js

>     Services.obs.addObserver(this, "Tab:Screenshot", false);
>+    Services.obs.addObserver(this, "Tab:ScreenshotNext", false);

I'd like to switch to use the thread manager and dispatching to continue the queue, so let's remove this, but you do need to add "Tab:Screenshot:Cancel"

>   screenshotTab: function screenshotTab(aData) {
>+      if (this.screenshotQueue == null) {
>+          this.screenShotQueue = new Array();

nit: use []
            this.screenShotQueue = [];

>+  doNextScreenshot: function() {
>+      if (this.screenshotQueue == null || this.screenshotQueue.length == 0) {
>+          this.screenshotQueue = null;
>+          return

nit: add ;

>+    } else if (aTopic == "Tab:ScreenshotNext") {
>+      this.doNextScreenshot();

Remove

>+    } else if (aTopic == "Tab:CancelScreenshots") {

"Tab:Screenshot:Cancel"

>       sendMessageToJava(message);
>+      Services.obs.notifyObservers(this, "Tab:ScreenshotNext", "");

This is kinda convenient, but it's also a bit of an abuse. Can we use this code instead:

Services.tm.mainThread.dispatch(function() {
  BrowserApp.doNextScreenshot()
}, Ci.nsIThread.DISPATCH_NORMAL);

r+, but I'll keep thinking of ways to make this impl more likable
Attachment #591604 - Flags: review?(mark.finkle) → review+
Like maybe making a simple JS class for the ScreenshotQueue instead of adding it to BrowserApp
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b1b20d9593
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/c9b1b20d9593
Assignee: nobody → blassey.bugs
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 591604 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
the gecko event loop can be bogged down by the screenshot creation even when is no longer needed
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
pretty safe, but if something were to go wrong, the we'd have blank thumbnails.
Attachment #591604 - Flags: approval-mozilla-aurora?

Comment 6

5 years ago
Comment on attachment 591604 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #591604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/45b862358c3b
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
status-firefox12: --- → fixed
You need to log in before you can comment on or make changes to this bug.