Closed Bug 910036 Opened 6 years ago Closed 6 years ago

about:newtab shouldn't load thumbnails in background when hidden by preloader

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

(Keywords: perf, regression, Whiteboard: [MemShrink][qa-])

Attachments

(1 file, 3 obsolete files)

Attached patch patch.diff (obsolete) — Splinter Review
about:newtab shouldn't load thumbnails in the background when it's hidden due to being loaded by the newtab preloader.  See bug 896912 comment 13, bug 901294.
Attachment #796393 - Flags: review?(ttaubert)
Comment on attachment 796393 [details] [diff] [review]
patch.diff

Review of attachment 796393 [details] [diff] [review]:
-----------------------------------------------------------------

In terms of having a solution that works in an e10s world, how about this:

We start off with a flag=!(browser.newtab.preload) that determines whether to call captureIfStale(). If preloading is disabled we can call it like usual. If preloading is enabled however we don't call it until the newtab preloader has sent us a message that makes us flip the flag. BrowserNewTabPreloader.newTab() is called for every tab opened with about:newtab as its initial URL. So before we even try to swap (which can fail if there's nothing preloaded, yet) we can send a message and kick off capturing stale thumbnails. As about:newtab is to become unprivileged, we should use the postMessage API here.

::: browser/modules/BrowserNewTabPreloader.jsm
@@ +326,5 @@
>      Array.forEach(scripts, script => mm.loadFrameScript(script, true));
>  
> +    // Allow the newly visible page to load thumbnails in the background.
> +    aTab.linkedBrowser.contentDocument.documentElement.classList.
> +      add("allowBackgroundCaptures");

This doesn't quite match the goal of making about:newtab more e10s friendly, unfortunately.
Attachment #796393 - Flags: review?(ttaubert)
Attached patch frame script + class mutation (obsolete) — Splinter Review
postMessage("allowBackgroundCaptures", targetOrigin, null) fails here, when it tries to modify the nsIURI it creates from targetOrigin:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7025

It fails whether targetOrigin is "about:newtab" or "chrome://browser/content/newtab/newTab.xul" because both the about and chrome protocol handlers make their URIs immutable:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#120
http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeProtocolHandler.cpp#107

Passing "*" skips the URI modifications and ends up working, but it's insecure, so we probably shouldn't do that.

This seems like a silly and fixable thing to prevent us from using postMessage, but postMessage isn't e10s-safe, is it?  You need a content window to call postMessage on, but with e10s, browser.contentWindow will be unavailable.  Will some window stub be available with e10s that I'm unaware of?  (I suppose eventually there will have to be in order not to break content that uses postMessage?)

So this patch uses a frame script, plus the class mutation from the earlier patch.  I'm open to using some mechanism other than the class mutation to communicate between the frame script and page, if you'd like.  I tested this with preloading both enabled and disabled.
Attachment #796393 - Attachment is obsolete: true
Attachment #797017 - Flags: review?(ttaubert)
There's nothing magical about postMessage wrt e10s, just that the response can be handled asynchronously, and so you're not baking in any assumptions about timing. But given that this is a simple one-way message, I think firing a DOM event would work fine too? Seems simpler to listen for (no need for a mutation observer).

Rather than a separate frame script, can we just add this processing to content.js?
You need a content window to call postMessage on.  How do you get a content window if the browser's remote?

DOM event would work.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Rather than a separate frame script, can we just add this processing to
> content.js?

I think so, but that's how we end up with an enormous browser.js that has everything shoved in it.  Yuck.
Duplicate of this bug: 907067
Comment on attachment 797017 [details] [diff] [review]
frame script + class mutation

Tim, will you be able to look at this soon?
Comment on attachment 797017 [details] [diff] [review]
frame script + class mutation

>diff --git a/browser/base/content/newtab/page.js b/browser/base/content/newtab/page.js

>+    this._mutationObserver = new MutationObserver(() => {
>+      if (this.allowBackgroundCaptures) {

Hrm, I guess you're assuming that the only "class" attribute change that will occur on this page is the removal/addition of allowBackgroundCaptures. I suppose that's fine, but it might be nicer to just use a custom attribute to avoid future problems.

Can you write a test for this?
Attachment #797017 - Flags: review?(ttaubert) → review+
Attached patch custom attribute + test (obsolete) — Splinter Review
Thanks, this uses a custom attribute and includes a test.

https://tbpl.mozilla.org/?tree=Try&rev=0a84b7aed867
Attachment #797017 - Attachment is obsolete: true
Same as previous, just with an hg header for easier landing.
Attachment #824845 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/d718d65cbe0b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d718d65cbe0b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Depends on: 933984
It looks like this fixed a memory usage regression (Tp5 "Private Bytes").  Should it be considered for uplift to Aurora?
Keywords: perf, regression
Whiteboard: [MemShrink]
(In reply to Drew Willcoxon :adw from comment #9)
> Created attachment 825058 [details] [diff] [review]
> custom attribute + test (for check-in)
> 
> Same as previous, just with an hg header for easier landing.

Please request approval on aurora if this is ready for uplift.
Comment on attachment 825058 [details] [diff] [review]
custom attribute + test (for check-in)

This patch applies cleanly to Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): background thumbnailing (bug 870100)
User impact if declined: extra resource usage for no apparent reason
Testing completed (on m-c, etc.): automated test on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #825058 - Flags: approval-mozilla-aurora?
Attachment #825058 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there any help needed for manual testing?
Flags: needinfo?(adw)
No.
Flags: needinfo?(adw)
Whiteboard: [MemShrink] → [MemShrink][qa-]
You need to log in before you can comment on or make changes to this bug.