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.

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.

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.

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.

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?

Thanks, this uses a custom attribute and includes a test. https://tbpl.mozilla.org/?tree=Try&rev=0a84b7aed867

Same as previous, just with an hg header for easier landing.

It looks like this fixed a memory usage regression (Tp5 "Private Bytes"). Should it be considered for uplift to Aurora?

(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

Is there any help needed for manual testing?

No.