The code added in bug 1347204 ends up eagerly loading LightweightThemeChildListener.jsm in every content process, even though it makes some pretense of doing it lazily. This leads to a 17K memory regression for every base content process. At this point, you should probably just wait for bug 1471025 to land, and use the sharedData map for the lightweight theme data, so AS can just fetch it when it's needed. But this definitely needs to be fixed.

I don't know if we can use the shared memory map from bug 1471025, simply because there's per-window theming to handle. However, I can definitively give a try to load LWTChildListener.jsm more lazily than now.

(In reply to Tim Nguyen :ntim from comment #1) > I don't know if we can use the shared memory map from bug 1471025, simply > because there's per-window theming to handle. I don't see why that would be a problem.

(In reply to Kris Maglione [:kmag] from comment #2) > (In reply to Tim Nguyen :ntim from comment #1) > > I don't know if we can use the shared memory map from bug 1471025, simply > > because there's per-window theming to handle. > > I don't see why that would be a problem. Theme data can't be shared between windows/processes if it's different based on the window.

(In reply to Tim Nguyen :ntim from comment #3) > (In reply to Kris Maglione [:kmag] from comment #2) > > (In reply to Tim Nguyen :ntim from comment #1) > > > I don't know if we can use the shared memory map from bug 1471025, simply > > > because there's per-window theming to handle. > > > > I don't see why that would be a problem. > > Theme data can't be shared between windows/processes if it's different based > on the window. Sure it can. It just needs to be keyed by window.

So, jotting down a few ideas here: 1. We should ensure that the LightweightThemeChildListener is only ever loaded in the privileged content process, once we turn that process on by default. That'd rely on bug 1472212 landing and sticking, but is probably a good idea in general - I highly doubt we'd ever want to expose this API to any pages outside of the privileged content process. 2. We could try to use the SharedData structure to make sure the content processes (or privileged content process, once it's turned on) are aware of all of the individual styles set per window, keyed on some kind of window identifier. I'm not sure if each TabChild currently has an easy way of determining which window they're in, but if not, we'd probably need to add it and expose it. The parent would be responsible for updating this structure any time the theme decides to change for one or more windows. 3. Keep the current general design, but make the LightweightThemeChildListener even lazier by only ever loading it if one of the about: pages it cares about is loaded (so, only load it in the event that the LightweightTheme:Support event is fired). If and when the message with the theme data comes down from the parent, have the stub hold onto it until that event appears.

I'd really rather it only ever be loaded when an about: page that requires it is open. Using sharedData is the easiest way to make that work. It allows us to synchronously fetch the theme data when we need it, and directly add a "change" listener if we need to care about dynamic updates. It should also make the code trivial enough that it can go into some other privileged script that we load for those pages, rather than requiring its own JSM. (In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #5) > I'm not sure if each TabChild currently has an easy way of > determining which window they're in, but if not, we'd probably need to add > it and expose it. The parent would be responsible for updating this > structure any time the theme decides to change for one or more windows. They don't, but it should be pretty easy to add. Ideally, we'd implement it natively, and update it whenever we attach a FrameLoader to a browser.

(In reply to Kris Maglione [:kmag] from comment #6) > (In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from > comment #5) > > I'm not sure if each TabChild currently has an easy way of > > determining which window they're in, but if not, we'd probably need to add > > it and expose it. The parent would be responsible for updating this > > structure any time the theme decides to change for one or more windows. > > They don't, but it should be pretty easy to add. Ideally, we'd implement it > natively, and update it whenever we attach a FrameLoader to a browser. I guess we might be able to just add it to https://searchfox.org/mozilla-central/source/dom/ipc/PTabContext.ipdlh ...

Sorry, but this is absolutely not P3. We can't afford a 17K content memory regression at this point. I've spent hours working to get a single 10K improvement in some areas. A 17K regression that isn't fixed is going to need to be backed out.

(In reply to Kris Maglione [:kmag] from comment #7) > (In reply to Kris Maglione [:kmag] from comment #6) > > (In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from > > comment #5) > > > I'm not sure if each TabChild currently has an easy way of > > > determining which window they're in, but if not, we'd probably need to add > > > it and expose it. The parent would be responsible for updating this > > > structure any time the theme decides to change for one or more windows. > > > > They don't, but it should be pretty easy to add. Ideally, we'd implement it > > natively, and update it whenever we attach a FrameLoader to a browser. > > I guess we might be able to just add it to > https://searchfox.org/mozilla-central/source/dom/ipc/PTabContext.ipdlh ... What about tab detaching ?

(In reply to Tim Nguyen :ntim from comment #9) > What about tab detaching ? During a frameloader swap, we'll want to message the TabChild(s) with the new IDs for the windows they belong to.

(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #10) > (In reply to Tim Nguyen :ntim from comment #9) > > What about tab detaching ? > > During a frameloader swap, we'll want to message the TabChild(s) with the > new IDs for the windows they belong to. We already update the TabContext on frameloader swap, which is why I suggested it.

Alright, let's see if we can get the SharedData approach to work. Here's a strawman breakdown: 1. See if we can send down a window identifier to each frameloader on construction, and ensure that the identifier is updated on frameloader swaps 2. Have the parent populate the SharedData storage with the dynamic themes as they update, keyed on the window identifiers 3. Have tab-content.js not load LightweightThemeChildListener until one of the about: pages that it cares about loads 4. When LightweightThemeChildListener is loaded, have it read from SharedData, and get the right theme values for the window that the tab belongs to. Send that information back to the page that's loading contentTheme.js 5. During frameloader swaps, have the LightweightThemeChildListener notice if the associated tab's window ID has changed, and if so, update the contentTheme.js script appropriately. Hey kmag, reading bug 1471025 (mainly the tests), does this mean we'd be using prefs as the backing store for the theme information?

(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #12) > Hey kmag, reading bug 1471025 (mainly the tests), does this mean we'd be > using prefs as the backing store for the theme information? Er, I guess I linked the wrong bug. I meant bug 1463587 :)

Probably most relevant are: https://reviewboard.mozilla.org/r/252558/diff/3#file8171396 https://reviewboard.mozilla.org/r/252562/diff/3#file8171424

Gotcha, gotcha - okay, yeah, we can totally use this. Thanks!

Once bug 1474662 merges, the outer window ID of the browser window will be available to frame scripts. I think what we'll probably want to do then is to have something in the parent process monitor for lightweight-theme-styling-update notifications, and then set the content-relevant properties in the SharedData structure. In tab-content.js, we should have something that only loads LightweightThemeChildListener.jsm if one of the about: pages in the whitelist is loaded. LightweightThemeChildListener.jsm should then wait for the content events for the theme requests, and send the information from the SharedData structure. It should also add a change event listener to the SharedData structure, and send LightweightTheme:Update events down to the about: page if the SharedData for the theme data changes. LightweightThemeChildListener should use the chromeOuterWindowID information exposed in bug 1474662 to choose the right theme properties. I think we should also modify contentTheme.js to request theme information from the LightweightThemeChildListener on the pageshow event. That way after frameloader swaps, it'll get the update without the parent having to tell it with an extra message (this means I believe we can back out bug 1473270). Hey ntim, do you think you'd have the time to take on this refactor in the 63 cycle?

(I'm happy to coach you through it and review)

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264006 Code analysis found 10 defects in this patch: - 10 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/tab-content.js:85 (Diff revision 1) > - }, > -}; > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { Error: Unexpected space before function parentheses. [eslint: space-before-function-paren] ::: browser/base/content/tab-content.js:87 (Diff revision 1) > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { > + if (originalTarget.defaultView == content) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:92 (Diff revision 1) > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:93 (Diff revision 1) > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:8 (Diff revision 1) > // This test checks whether the new tab page color properties work. > > + > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. Error: Expected JSDoc for 'win' but found 'the'. [eslint: valid-jsdoc] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:11 (Diff revision 1) > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { Error: 'waitForSharedDataChange' is defined but never used. Allowed unused vars must match /^console$/. [eslint: no-unused-vars] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 1) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 1) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space before '}'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 1) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 1) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space before '}'. [eslint: object-curly-spacing]

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264008 Code analysis found 10 defects in this patch: - 10 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/tab-content.js:85 (Diff revision 2) > - }, > -}; > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { Error: Unexpected space before function parentheses. [eslint: space-before-function-paren] ::: browser/base/content/tab-content.js:87 (Diff revision 2) > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { > + if (originalTarget.defaultView == content) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:92 (Diff revision 2) > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:93 (Diff revision 2) > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:8 (Diff revision 2) > // This test checks whether the new tab page color properties work. > > + > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. Error: Expected JSDoc for 'win' but found 'the'. [eslint: valid-jsdoc] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:11 (Diff revision 2) > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { Error: 'waitForSharedDataChange' is defined but never used. Allowed unused vars must match /^console$/. [eslint: no-unused-vars] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 2) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 2) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space before '}'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 2) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 2) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space before '}'. [eslint: object-curly-spacing]

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264086 ::: browser/base/content/tab-content.js:86 (Diff revision 2) > -}; > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { > + if (originalTarget.defaultView == content) { Let's move the whitelist into this file, and only do the \_update (and therefore only load the JSM) once an entry from that whitelist is loaded. ::: browser/base/content/tab-content.js:91 (Diff revision 2) > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}); Perhaps this is another thing we should only do once the JSM has been loaded. ::: browser/modules/LightweightThemeChildHelper.jsm:36 (Diff revision 2) > /** > * Forward the theme data to the page. > - * @param {Object} data The theme data to forward > + * @param {Object} outerWindowId The outerWindowId the parent process window has. > * @param {Object} content The receiving global > */ > - _update(data, content) { > + _update(outerWindowId, content) { This can be, I think, gotten from the content (via content.chromeOuterWindowID). So I don't think you necessarily need the extra argument here. ::: browser/modules/LightweightThemeChildHelper.jsm:49 (Diff revision 2) > } > + }, > - } > +} > -} > > -var EXPORTED_SYMBOLS = ["LightweightThemeChildListener"]; > +var EXPORTED_SYMBOLS = ["LightweightThemeChildHelper"]; Nit: I forget why we put this down here. I think it's convention to put these EXPORTED_SYMBOLS lists at the top - let's do that.

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264090 Code analysis found 11 defects in this patch: - 11 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/contentTheme.js:71 (Diff revision 3) > - this._setProperties(document.body, data); > + this._setProperties(document.body, data); > + break; > + case "pageshow": > + const event = new CustomEvent("LightweightTheme:Support", {bubbles: true}); > + document.dispatchEvent(event); > + console.log("pageshow hi") Error: Missing semicolon. [eslint: semi] ::: browser/base/content/tab-content.js:85 (Diff revision 3) > - }, > -}; > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { Error: Unexpected space before function parentheses. [eslint: space-before-function-paren] ::: browser/base/content/tab-content.js:87 (Diff revision 3) > - > -LightweightThemeChildListenerStub.init(); > > +addEventListener("LightweightTheme:Support", function ({ originalTarget }) { > + if (originalTarget.defaultView == content) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:92 (Diff revision 3) > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:93 (Diff revision 3) > + } > +}, false, true); > + > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:8 (Diff revision 3) > // This test checks whether the new tab page color properties work. > > + > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. Error: Expected JSDoc for 'win' but found 'the'. [eslint: valid-jsdoc] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:11 (Diff revision 3) > +/** > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { Error: 'waitForSharedDataChange' is defined but never used. Allowed unused vars must match /^console$/. [eslint: no-unused-vars] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 3) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:12 (Diff revision 3) > + * Wait for a change from the sharedData structure. > + * @param {Object} the window that should have its theme changed. > + * @returns {Promise} the promise to wait for. > + */ > +function waitForSharedDataChange(win) { > + const { outerWindowID } = win Error: There should be no space before '}'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 3) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space after '{'. [eslint: object-curly-spacing] ::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:18 (Diff revision 3) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + > + const changedKey = `theme/${outerWindowID}`; > + return new Promise(function(resolve) { > + let listener = ({ changedKeys }) => { Error: There should be no space before '}'. [eslint: object-curly-spacing]

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264086 > This can be, I think, gotten from the content (via content.chromeOuterWindowID). So I don't think you necessarily need the extra argument here. Don't think so. You'd have to map the content window to its frameloader, and get the property from that. Which can be done, but gets a bit verbose...

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264086 > Don't think so. You'd have to map the content window to its frameloader, and get the property from that. Which can be done, but gets a bit verbose... Ach, whoops - you're right. I confused content with the global. Nevermind me.

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264152 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/tab-content.js:96 (Diff revision 5) > - }, > > - receiveMessage(msg) { > - return this.childListener.receiveMessage(msg); > - }, > -}; > +addEventListener("LightweightTheme:Support", function({ originalTarget }) { > + if (originalTarget.defaultView == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + lastThemablePage = content; > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:102 (Diff revision 5) > + } > +}, false, true); > > -LightweightThemeChildListenerStub.init(); > +addEventListener("pageshow", function() { > + if (lastThemablePage == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:107 (Diff revision 5) > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}); > > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`) && Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef] ::: browser/base/content/tab-content.js:109 (Diff revision 5) > +}); > > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`) && > + lastThemablePage == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); Error: 'chromeOuterWindowID' is not defined. [eslint: no-undef]

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264180 ::: browser/base/content/tab-content.js:100 (Diff revision 5) > -LightweightThemeChildListenerStub.init(); > +addEventListener("pageshow", function() { > + if (lastThemablePage == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}); > > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`) && > + lastThemablePage == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}); This should all happen in LighweightThemeChildHelper.jsm after the first Support event has been received. It doesn't serve any purpose before then, and adds non-trivial memory overhead..

Reviewing today - just freeing up from meetings.

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264784 Hey ntim, this is a solid approach - I have some minor suggestions, but this looks like it's on the right track. See below! Thanks! ::: browser/base/content/tab-content.js:91 (Diff revision 6) > - addMessageListener("LightweightTheme:Update", this); > - sendAsyncMessage("LightweightTheme:Request"); > - this.init = null; > + "about:newtab", > + "about:welcome", > +]); > - }, > > - handleEvent(event) { > +let lastThemablePage; Hm. This looks like a good way of leaking DOM windows... this doesn't ever get cleared, it seems. ::: browser/base/content/tab-content.js:93 (Diff revision 6) > > - handleEvent(event) { > +let lastThemablePage; > - return this.childListener.handleEvent(event); > - }, > > - receiveMessage(msg) { > +addEventListener("LightweightTheme:Support", function({ originalTarget }) { So at first glance, this is good - the event will be coming in on the pages that have contentTheme.js, and then we check the content documentURI against the whitelist. So I think this is a sufficient way of knowing when to finally load the JSM. However, I notice that the pageshow event handler is essentially doing the same thing. Also, the pageshow event handler should fire in the original not long after loading contentTheme.js... so perhaps we can just have a pageshow event handler here instead that checks against the whitelist, and get rid of the LightweightTheme:Support event? ::: browser/base/content/tab-content.js:106 (Diff revision 6) > +Services.cpmm.sharedData.addEventListener("change", function({ changedKeys }) { > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`) && > + lastThemablePage == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + LightweightThemeChildHelper._update(chromeOuterWindowID, content); > + } > +}); This is another thing that probably only needs to happen inside of the JSM. Either have the JSM (again) export a class that can be instantiated per global / chromeOuterWindowID, and have it monitor for the appropriate change event.

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review264784 > This is another thing that probably only needs to happen inside of the JSM. Either have the JSM (again) export a class that can be instantiated per global / chromeOuterWindowID, and have it monitor for the appropriate change event. If the memory overhead of instantiating an instance of that class per top-level content is prohibitive, another idea would be to have the JSM monitor for changes on the theme/ namespace, and then iterate all top-level DOM windows, searching for ones pointed at pages in the whitelist, and then updating them. You can iterate windows like this: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/devtools/server/actors/targets/content-process.js#41-48

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review265392 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/tab-content.js:93 (Diff revision 7) > - > +]); > -LightweightThemeChildListenerStub.init(); > > +addEventListener("pageshow", function({ originalTarget }) { > + if (originalTarget.defaultView == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + console.log(content.document.documentURI) Error: Missing semicolon. [eslint: semi]

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review265394 Hey ntim, I think this is looking pretty good - but needs a tiny bit of cleanup (there's some debugging stuff in there, and I have some naming / organizing suggestions). Thanks! ::: browser/base/content/tab-content.js:85 (Diff revision 7) > - receiveMessage(msg) { > - return this.childListener.receiveMessage(msg); > - }, > -}; > - > +let themeablePagesWhitelist = new Set([ > + "about:home", > + "about:newtab", > + "about:welcome", > +]); What about storing this as a property on E10SUtils? That way, you don't have to pass it into LightweightThemeChildHelper. ::: browser/base/content/tab-content.js:93 (Diff revision 7) > - > +]); > -LightweightThemeChildListenerStub.init(); > > +addEventListener("pageshow", function({ originalTarget }) { > + if (originalTarget.defaultView == content && themeablePagesWhitelist.has(content.document.documentURI)) { > + console.log(content.document.documentURI) Please remove this logging. ::: browser/modules/LightweightThemeChildHelper.jsm:20 (Diff revision 7) > - if (name == "LightweightTheme:Update") { > - this._lastData = data; > - this._update(data, target.content); > - } > + listener: null, > + whitelist: [], > + _listen(whitelist) { > + if (!this.listener) { > + this.whitelist = whitelist; > + console.log("WHITELIST", whitelist) Please remove this, and the rest of the console.log logging. ::: browser/modules/LightweightThemeChildHelper.jsm:33 (Diff revision 7) > + while (windowEnumerator.hasMoreElements()) { > + const window = windowEnumerator.getNext().QueryInterface(Ci.nsIDOMWindow); > + const tabChildGlobal = window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDocShell) > + .sameTypeRootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIContentFrameMessageManager); > + const {chromeOuterWindowID, content} = tabChildGlobal; > + if (changedKeys.includes(`theme/${chromeOuterWindowID}`) && > + content && this.whitelist.has(content.document.documentURI)) { > + this._update(chromeOuterWindowID, content); > - } > + } Going back to a previous conversation we had - I guess there is some risk here that this could impact performance immediately after window opens (since we'll add a new entry on window opening, and then iterate all tabs in all content processes searching for applicable content). If this ends up being a problem, two ideas come to mind: 1. We could hold some kind of weak list of pages that are at URLs in the whitelist, and iterate through those instead. We'd need to manage that whitelist, and that might add a bit of memory overhead - space/time trade-off there. 2. We could break up this loop to occur in one or more idle ticks. Any performance impact here is speculation, though. Let's not prematurely optimize - let's see how much this actually impacts things in practice. ::: browser/modules/LightweightThemeChildHelper.jsm:50 (Diff revision 7) > - } > + } > + }, > > /** > * Forward the theme data to the page. > - * @param {Object} data The theme data to forward > + * @param {Object} outerWindowId The outerWindowId the parent process window has. nit: outerWindowID for consistency. ::: browser/modules/LightweightThemeChildHelper.jsm:53 (Diff revision 7) > /** > * Forward the theme data to the page. > - * @param {Object} data The theme data to forward > + * @param {Object} outerWindowId The outerWindowId the parent process window has. > * @param {Object} content The receiving global > */ > - _update(data, content) { > + _update(outerWindowId, content) { Let's just call this update, since it's called by an external consumer.

Holding off on reviewing until most recent try comes in (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9df2fad745eff80b363e95a51ee8778cf9507f0b)

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review265652 Thanks!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f840add2cdd4 Make use of sharedData for content theme data. r=mconley

Backed out for causing leaks. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=f840add2cdd448ccb2a59d97c0ac6cbd967f038d&tochange=e207f210919a58c9032e3ea032ee1553194c8872&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=mochitest&selectedJob=189525571 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189525571&repo=autoland&lineNumber=10572 https://treeherder.mozilla.org/logviewer.html#?job_id=189525571&repo=autoland&lineNumber=10572 https://treeherder.mozilla.org/logviewer.html#?job_id=189514473&repo=autoland&lineNumber=3843 Backout link: https://hg.mozilla.org/integration/autoland/rev/e207f210919a58c9032e3ea032ee1553194c8872

Comment on attachment 8992325 [details] Bug 1474163 - Make use of sharedData for content theme data. https://reviewboard.mozilla.org/r/257196/#review265768 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/tab-content.js:106 (Diff revision 15) > + LightweightThemeChildHelper.listen(themeablePagesWhitelist); > + LightweightThemeChildHelper.update(chromeOuterWindowID, content); > + } > +}, false, true); > > +addEventListener("pagehide", function ({ originalTarget }) { Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7fd40900b7c1 Make use of sharedData for content theme data. r=mconley

Backed out changeset 7fd40900b7c1 (bug 1474163) for assertion failure at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1070 on a CLOSED TREE Failing push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7fd40900b7c1d576d7ff3f1c7bfda330ca740ca8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189574832&repo=autoland&lineNumber=2726 10:36:27 INFO - GECKO(826) | Assertion failure: value, at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1070 10:37:15 INFO - GECKO(826) | #01: mozilla::dom::ContentProcessMessageManager_Binding::get_sharedData(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProcessGlobal*, JSJitGetterCallArgs) [dom/bindings/BindingUtils.h:0] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #02: bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:3189] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #03: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/vm/Interpreter.cpp:444] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #04: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:532] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #05: js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:602] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #06: bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) [js/src/vm/NativeObject.cpp:2149] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #07: bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) [js/src/vm/NativeObject.cpp:2415] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #08: js::ForwardingProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const [js/public/RootingAPI.h:1007] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #09: js::CrossCompartmentWrapper::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const [js/src/proxy/CrossCompartmentWrapper.cpp:226] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #10: js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:351] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #11: js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) [js/public/RootingAPI.h:1007] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #12: js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) [js/public/RootingAPI.h:1007] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #13: js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) [js/public/RootingAPI.h:1007] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #14: Interpret(JSContext*, js::RunState&) [js/public/RootingAPI.h:1007] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #15: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:424] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #16: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:556] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #17: <name omitted> [js/src/vm/Interpreter.cpp:602] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #18: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2829] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #19: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) [js/xpconnect/src/XPCWrappedJSClass.cpp:1144] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | #20: PrepareAndDispatch [xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_darwin.cpp:0] 10:37:15 INFO - 10:37:15 INFO - GECKO(826) | --DOCSHELL 0x11e412800 == 0 [pid = 829] [id = {d58f6f24-d6b2-fc48-a7d7-ff5e815b26d9}] 10:37:15 INFO - GECKO(826) | nsStringStats 10:37:15 INFO - GECKO(826) | => mAllocCount: 13235 10:37:15 INFO - GECKO(826) | => mReallocCount: 504 10:37:15 INFO - GECKO(826) | => mFreeCount: 13235 10:37:15 INFO - GECKO(826) | => mShareCount: 11227 10:37:15 INFO - GECKO(826) | => mAdoptCount: 1385 10:37:15 INFO - GECKO(826) | => mAdoptFreeCount: 1393 10:37:15 INFO - GECKO(826) | => Process ID: 831, Thread ID: 140735264809728 10:37:15 INFO - GECKO(826) | nsStringStats 10:37:15 INFO - GECKO(826) | => mAllocCount: 15768 10:37:15 INFO - GECKO(826) | => mReallocCount: 571 10:37:15 INFO - GECKO(826) | => mFreeCount: 15768 10:37:15 INFO - GECKO(826) | => mShareCount: 13311 10:37:15 INFO - GECKO(826) | => mAdoptCount: 1654 10:37:15 INFO - GECKO(826) | => mAdoptFreeCount: 1678 10:37:15 INFO - GECKO(826) | => Process ID: 830, Thread ID: 140735264809728 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 1 (0x10d841c00) [pid = 829] [serial = 10] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | [Child 829, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 517 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 0 (0x11f65e400) [pid = 829] [serial = 11] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | nsStringStats 10:37:15 INFO - GECKO(826) | => mAllocCount: 14779 10:37:15 INFO - GECKO(826) | => mReallocCount: 552 10:37:15 INFO - GECKO(826) | => mFreeCount: 14779 10:37:15 INFO - GECKO(826) | => mShareCount: 11926 10:37:15 INFO - GECKO(826) | => mAdoptCount: 1778 10:37:15 INFO - GECKO(826) | => mAdoptFreeCount: 1786 10:37:15 INFO - GECKO(826) | => Process ID: 829, Thread ID: 140735264809728 10:37:15 INFO - GECKO(826) | 1532367387893 Marionette DEBUG Received observer notification xpcom-will-shutdown 10:37:15 INFO - GECKO(826) | 1532367387895 Marionette INFO Stopped listening on port 2828 10:37:15 INFO - GECKO(826) | 1532367387895 Marionette DEBUG Remote service is inactive 10:37:15 INFO - GECKO(826) | [Parent 826, Main Thread] WARNING: NS_ENSURE_TRUE(mDB) failed: file /builds/worker/workspace/build/src/netwerk/cache/nsDiskCacheDeviceSQL.cpp, line 1422 10:37:15 INFO - GECKO(826) | [Parent 826, Main Thread] WARNING: nsAppShell::Exit() called redundantly: file /builds/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 758 10:37:15 INFO - GECKO(826) | [Parent 826, Main Thread] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp, line 909 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 9 (0x124851800) [pid = 826] [serial = 2] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 8 (0x121821c00) [pid = 826] [serial = 4] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 7 (0x1247cb000) [pid = 826] [serial = 3] [outer = 0x0] [url = chrome://browser/content/browser.xul] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 6 (0x12525c200) [pid = 826] [serial = 13] [outer = 0x0] [url = chrome://mochikit/content/browser-harness.xul] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 5 (0x135c12c00) [pid = 826] [serial = 14] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 4 (0x1303dfc00) [pid = 826] [serial = 9] [outer = 0x0] [url = chrome://extensions/content/dummy.xul] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 3 (0x132384000) [pid = 826] [serial = 12] [outer = 0x0] [url = chrome://extensions/content/dummy.xul] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 2 (0x10f05c400) [pid = 826] [serial = 1] [outer = 0x0] [url = chrome://browser/content/hiddenWindow.xul] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 1 (0x12e128e00) [pid = 826] [serial = 5] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | --DOMWINDOW == 0 (0x124d43c00) [pid = 826] [serial = 16] [outer = 0x0] [url = about:blank] 10:37:15 INFO - GECKO(826) | nsStringStats 10:37:15 INFO - GECKO(826) | => mAllocCount: 91407 10:37:15 INFO - GECKO(826) | => mReallocCount: 7717 10:37:15 INFO - GECKO(826) | => mFreeCount: 91407 10:37:15 INFO - GECKO(826) | => mShareCount: 103042 10:37:15 INFO - GECKO(826) | => mAdoptCount: 3834 10:37:15 INFO - GECKO(826) | => mAdoptFreeCount: 3896 10:37:15 INFO - GECKO(826) | => Process ID: 826, Thread ID: 140735264809728 10:37:15 INFO - TEST-INFO | Main app process: exit 0 10:37:15 INFO - runtests.py | Application ran for: 0:01:18.065940 10:37:15 INFO - zombiecheck | Reading PID log: /var/folders/xw/5zcdsm497bqfh05smhlpzpf000000w/T/tmpmooR48pidlog 10:37:15 INFO - ==> process 826 launched child process 827 10:37:15 INFO - ==> process 826 launched child process 828 10:37:15 INFO - ==> process 826 launched child process 829 10:37:15 INFO - ==> process 826 launched child process 830 10:37:15 INFO - ==> process 826 launched child process 831 10:37:15 INFO - zombiecheck | Checking for orphan process with PID: 827 10:37:15 INFO - zombiecheck | Checking for orphan process with PID: 828 10:37:15 INFO - zombiecheck | Checking for orphan process with PID: 829 10:37:15 INFO - zombiecheck | Checking for orphan process with PID: 830 10:37:15 INFO - zombiecheck | Checking for orphan process with PID: 831 10:37:15 INFO - mozcrash Copy/paste: /Users/cltbld/tasks/task_1532367055/build/macosx64-minidump_stackwalk /var/folders/xw/5zcdsm497bqfh05smhlpzpf000000w/T/tmpelpMXl.mozrunner/minidumps/0D7ABB4C-3342-4E73-8A04-7FC09C1F00D6.dmp /Users/cltbld/tasks/task_1532367055/build/symbols 10:37:32 INFO - mozcrash Saved minidump as /Users/cltbld/tasks/task_1532367055/build/blobber_upload_dir/0D7ABB4C-3342-4E73-8A04-7FC09C1F00D6.dmp 10:37:32 INFO - mozcrash Saved app info as /Users/cltbld/tasks/task_1532367055/build/blobber_upload_dir/0D7ABB4C-3342-4E73-8A04-7FC09C1F00D6.extra 10:37:32 INFO - PROCESS-CRASH | Main app process exited normally | application crashed [@ bool mozilla::dom::binding_detail::DoGetOrCreateDOMReflector<mozilla::dom::ipc::SharedMap, (mozilla::dom::binding_detail::GetOrCreateReflectorWrapBehavior)0>(JSContext*, mozilla::dom::ipc::SharedMap*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)] 10:37:32 INFO - Crash dump filename: /var/folders/xw/5zcdsm497bqfh05smhlpzpf000000w/T/tmpelpMXl.mozrunner/minidumps/0D7ABB4C-3342-4E73-8A04-7FC09C1F00D6.dmp

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/590299c07f06 Make use of sharedData for content theme data. r=mconley

Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b964d8badd9b Backed out changeset 7fd40900b7c1 for assertion failure at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1070 on a CLOSED TREE

Backed out changeset 590299c07f06 (bug 1474163) for AddressSanitizer failures on a CLOSED TREE. Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=41eac2adbadbb2a0cf5e98648d4db108054019e3&selectedJob=189578657 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189578657&repo=autoland&lineNumber=1746 [task 2018-07-23T18:14:22.512Z] 18:14:22 ERROR - GECKO(1044) | ==1123==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f0b912320f0 bp 0x7ffcbfdbcb10 sp 0x7ffcbfdbca40 T0) [task 2018-07-23T18:14:22.512Z] 18:14:22 INFO - GECKO(1044) | ==1123==The signal is caused by a READ memory access. [task 2018-07-23T18:14:22.513Z] 18:14:22 INFO - GECKO(1044) | ==1123==Hint: address points to the zero page. [task 2018-07-23T18:14:22.817Z] 18:14:22 INFO - GECKO(1044) | 1532369662815 Marionette DEBUG Received observer notification xpcom-will-shutdown [task 2018-07-23T18:14:22.819Z] 18:14:22 INFO - GECKO(1044) | 1532369662816 Marionette INFO Stopped listening on port 2828 [task 2018-07-23T18:14:22.821Z] 18:14:22 INFO - GECKO(1044) | 1532369662816 Marionette DEBUG Remote service is inactive [task 2018-07-23T18:14:23.389Z] 18:14:23 INFO - GECKO(1044) | #0 0x7f0b912320ef in GetWrapperPreserveColor /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:16:13 [task 2018-07-23T18:14:23.389Z] 18:14:23 INFO - GECKO(1044) | #1 0x7f0b912320ef in nsWrapperCache::GetWrapper() const /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:31 [task 2018-07-23T18:14:23.407Z] 18:14:23 INFO - GECKO(1044) | #2 0x7f0b94c8ef06 in DoGetOrCreateDOMReflector<mozilla::dom::ipc::SharedMap, mozilla::dom::binding_detail::eWrapIntoContextCompartment> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1072:26 [task 2018-07-23T18:14:23.407Z] 18:14:23 INFO - GECKO(1044) | #3 0x7f0b94c8ef06 in GetOrCreateDOMReflector<mozilla::dom::ipc::SharedMap> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1148 [task 2018-07-23T18:14:23.407Z] 18:14:23 INFO - GECKO(1044) | #4 0x7f0b94c8ef06 in GetOrCreate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1750 [task 2018-07-23T18:14:23.407Z] 18:14:23 INFO - GECKO(1044) | #5 0x7f0b94c8ef06 in GetOrCreateDOMReflector<RefPtr<mozilla::dom::ipc::SharedMap> > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1772 [task 2018-07-23T18:14:23.407Z] 18:14:23 INFO - GECKO(1044) | #6 0x7f0b94c8ef06 in mozilla::dom::ContentProcessMessageManager_Binding::get_sharedData(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProcessGlobal*, JSJitGetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/MessageManagerBinding.cpp:3132 [task 2018-07-23T18:14:23.409Z] 18:14:23 INFO - GECKO(1044) | #7 0x7f0b96a60a2e in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3187:13 [task 2018-07-23T18:14:23.415Z] 18:14:23 INFO - GECKO(1044) | #8 0x7f0b9cd9c16e in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:444:15 [task 2018-07-23T18:14:23.415Z] 18:14:23 INFO - GECKO(1044) | #9 0x7f0b9cd9c16e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:532 [task 2018-07-23T18:14:23.416Z] 18:14:23 INFO - GECKO(1044) | #10 0x7f0b9cd9fb25 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:583:12 [task 2018-07-23T18:14:23.416Z] 18:14:23 INFO - GECKO(1044) | #11 0x7f0b9cd9fb25 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:602 [task 2018-07-23T18:14:23.417Z] 18:14:23 INFO - GECKO(1044) | #12 0x7f0b9cd9fb25 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:722 [task 2018-07-23T18:14:23.434Z] 18:14:23 INFO - GECKO(1044) | #13 0x7f0b9de08c44 in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2149:16 [task 2018-07-23T18:14:23.434Z] 18:14:23 INFO - GECKO(1044) | #14 0x7f0b9de08c44 in GetExistingProperty<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2202 [task 2018-07-23T18:14:23.434Z] 18:14:23 INFO - GECKO(1044) | #15 0x7f0b9de08c44 in NativeGetPropertyInline<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2415 [task 2018-07-23T18:14:23.435Z] 18:14:23 INFO - GECKO(1044) | #16 0x7f0b9de08c44 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2451 [task 2018-07-23T18:14:23.436Z] 18:14:23 INFO - GECKO(1044) | #17 0x7f0b9da8cd02 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1692:12 [task 2018-07-23T18:14:23.436Z] 18:14:23 INFO - GECKO(1044) | #18 0x7f0b9da8cd02 in js::ForwardingProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:154 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #19 0x7f0b9da4fa30 in js::CrossCompartmentWrapper::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:226:23 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #20 0x7f0b9da62628 in getInternal /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:351:21 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #21 0x7f0b9da62628 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:361 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #22 0x7f0b9cda9313 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1691:16 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #23 0x7f0b9cda9313 in GetProperty /builds/worker/workspace/build/src/js/src/vm/JSObject.h:787 [task 2018-07-23T18:14:23.441Z] 18:14:23 INFO - GECKO(1044) | #24 0x7f0b9cda9313 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4579 [task 2018-07-23T18:14:23.458Z] 18:14:23 INFO - GECKO(1044) | #25 0x7f0b9cd89774 in GetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:218:12 [task 2018-07-23T18:14:23.458Z] 18:14:23 INFO - GECKO(1044) | #26 0x7f0b9cd89774 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2954 [task 2018-07-23T18:14:23.459Z] 18:14:23 INFO - GECKO(1044) | #27 0x7f0b9cd6ca8a in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:424:12 [task 2018-07-23T18:14:23.459Z] 18:14:23 INFO - GECKO(1044) | #28 0x7f0b9cd9ca44 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:556:15 [task 2018-07-23T18:14:23.459Z] 18:14:23 INFO - GECKO(1044) | #29 0x7f0b9cd9dfd2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:602:10 [task 2018-07-23T18:14:23.480Z] 18:14:23 INFO - GECKO(1044) | #30 0x7f0b9d9a782d in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2829:12 [task 2018-07-23T18:14:23.480Z] 18:14:23 INFO - GECKO(1044) | #31 0x7f0b92b3a076 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:1123:23 [task 2018-07-23T18:14:23.481Z] 18:14:23 INFO - GECKO(1044) | #32 0x7f0b91236c68 in PrepareAndDispatch /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:127:37 [task 2018-07-23T18:14:23.481Z] 18:14:23 INFO - GECKO(1044) | #33 0x7f0b91235b3a in SharedStub (/builds/worker/workspace/build/application/firefox/libxul.so+0x214fb3a) [task 2018-07-23T18:14:23.482Z] 18:14:23 INFO - GECKO(1044) | #34 0x7f0b910f2293 in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /builds/worker/workspace/build/src/xpcom/ds/nsObserverList.cpp:112:19 [task 2018-07-23T18:14:23.483Z] 18:14:23 INFO - GECKO(1044) | #35 0x7f0b910f5bc2 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /builds/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp:295:19 [task 2018-07-23T18:14:23.492Z] 18:14:23 INFO - GECKO(1044) | #36 0x7f0b91262646 in mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp:885:24 [task 2018-07-23T18:14:23.492Z] 18:14:23 INFO - GECKO(1044) | #37 0x7f0b9caccabc in XRE_TermEmbedding() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:230:3 [task 2018-07-23T18:14:23.492Z] 18:14:23 INFO - GECKO(1044) | #38 0x7f0b92141575 in mozilla::ipc::ScopedXREEmbed::Stop() /builds/worker/workspace/build/src/ipc/glue/ScopedXREEmbed.cpp:108:5 [task 2018-07-23T18:14:23.492Z] 18:14:23 INFO - GECKO(1044) | #39 0x7f0b9cacd578 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:768:16 [task 2018-07-23T18:14:23.500Z] 18:14:23 INFO - GECKO(1044) | #40 0x4f2284 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30 [task 2018-07-23T18:14:23.500Z] 18:14:23 INFO - GECKO(1044) | #41 0x4f2284 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287 [task 2018-07-23T18:14:23.561Z] 18:14:23 INFO - GECKO(1044) | #42 0x7f0bb149d82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291 [task 2018-07-23T18:14:23.561Z] 18:14:23 INFO - GECKO(1044) | #43 0x4216b8 in _start (/builds/worker/workspace/build/application/firefox/firefox+0x4216b8) [task 2018-07-23T18:14:23.561Z] 18:14:23 INFO - GECKO(1044) | AddressSanitizer can not provide additional info. [task 2018-07-23T18:14:23.561Z] 18:14:23 INFO - GECKO(1044) | SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:16:13 in GetWrapperPreserveColor

Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/07414f000aa1 Backed out changeset 590299c07f06 for AddressSanitizer failures on a CLOSED TREE.

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/565b62023b58 Make use of sharedData for content theme data. r=mconley

Can you please add some STRs for QA(and a test webextension if possible) or add the "qe-verify-" flag ?