1141041 - [e10s] Opening the "View" > "Page Style" menu in remote browser causes unsafe CPOW usage warnings

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Ian Moody [:Kwan] (UTC+1)]

+++ This bug was initially created as a clone of Bug #1133577 +++

STR:

1) Visit a site in an e10s window
2) Open the top-level "View" menu, then the "Page Style" submenu.

This causes some "unsafe CPOW usage" warnings in the Browser Console on most sites, but a large number of warnings on sites that offer a alternate set.

Actually switching styles appears to be fine, and doesn't cause any additional warnings.

All the warnings are in browser/base/content/browser.js and seem to be any use of getStyleSheetInfo() or its resulting value.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Still around.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hoping I can steal this one too.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

So my original plan to fix this bug was to have the content send up a list of stylesheets every time it completed a page load so that the parent could cache it - that way, the parent could access the cache synchronously when populating the Page Style menu. This, unfortunately, doesn't take into account dynamically added stylesheets, so I was going to use the StyleSheetAdded (and StyleSheetRemoved, StyleSheetChanged) events to detect changes to the stylesheet collections to invalidate and update the cache.

It turns out that those StyleSheet events are not enabled by default - one needs to set styleSheetChangeEventsEnabled on the nsIDocument in question for the events to be fired - it looks like this is because there's a bit of a perf hit[1].

So I abandoned that plan, and tried looking at the DOMLink events instead. Unfortunately, it looks like those events are not fired at all for stylesheets (see bug 1210074).

So I abandoned the cache idea entirely, and decided to try to asynchronously populate the Page Style menu after it had been opened. Essentially, when detecting the popupshowing event on the Page Style popup, I'd send a message down to content asking for the stylesheet info, and when the message came back (if the same browser was selected), we'd inject the right nodes into the popupmenu.

I've hit a bit of a snag with this last approach, because while I've got all of the infrastructure in place to populate the menu asynchronously, at least on OS X, the native global menu doesn't seem to become invalidated / updated. So now I'm down in the OS X menu glue trying to figure out if there's some what I can trigger an invalidation, but that'd be getting pretty hacky.



[1]: Here's a comparison view of normal Nightly, and a Nightly that fires those StyleSheet events all of the time. There's a 2.41% dromaeo css regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=48206c424625&newProject=try&newRevision=f80b9f6eea40 . I'm also seeing potential CART and DAMP regressions in there as well.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Another solution I've thought of is to not support dynamically added stylesheets in our alternative stylesheet implementation. That would mean that we wouldn't need the DOMLink or StyleSheet events - we'd just need to notice when the DOM for all frames have loaded, and then scan the stylesheets and send that up to the parent to cache.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

The other option is to just stop supporting alternative stylesheets altogether, which would certainly be the simplest option.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - [WIP] Stop using CPOWs for the Page Style menu.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I think the strategy I'm adopting here is going to slightly break some expectations about what the global getAllStyleSheets() function will return, which is a function that addons seem to use (according to mxr).

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Looks like I have some failing tests to figure out.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r?Gijs

The content process sends up the stylesheet info after the pageshow event,
so we should wait for that instead of load.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8669254 [details]
MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

https://reviewboard.mozilla.org/r/21159/#review19107

Comment 14

[:Gijs (he/him)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

https://reviewboard.mozilla.org/r/21131/#review19105

Hm, there's a number of issues here so I'd like to see this again.

Also, here's a random thing: from looking at this implementation I think bug 213289 (!! 12-year-old bug alert) might have been fixed somewhere along the line? At least, reading that bug the other day (long story) I suspected the implementation used indices into the stylesheet array, and this doesn't seem to do so... If so, please feel invited to close that bug. :-)

::: browser/base/content/browser.js:5848
(Diff revision 2)
>    getAllStyleSheets: function () {

So, I could be wrong, but it seems there are no callers of this inside the tree, and very few add-on callers. All of the ones relying on the forwarding behaviour from here:

https://dxr.mozilla.org/mozilla-central/rev/3d7532ce81ac571962abc3b99582fe7f5d685192/browser/base/content/browser.js#5938

look like they expect they can pass a content window. Clearly that hasn't been possble for a while now, and I think e10s makes it impossible to do so sensibly.

The other issue with it that now this will return an empty array until after pageshow. This broke a test, and I imagine it might break usecases from add-ons, especially if any of them are doing any kind of caching.

And of course, we now no longer have CPOWs for the stylesheet objects, but just wrappers with a fraction of the info.

If this method is really unused inside the tree, I'd like to use the opportunity awarded by us breaking compat anyway, to remove it and its forwarding stuff (see above), and create a differently-named function that takes a browser (default to gBrowser.selectedBrowser), calls `_getStyleSheetInfo` and returns its `filteredStyleSheets` property. That should serve add-ons' needs (that or they can write their own content/frame script).

Does that sound sensible?


Huh, reading more here, it even looks as if allStyleSheets is only used by this thing. Can we add the href to the list of properties for filteredStyleSheets and then kill off allStyleSheets altogether? Did I miss a use for allStyleSheets?

::: browser/base/content/browser.js:5856
(Diff revision 2)
>    _getStyleSheetInfo: function (browser) {
> -    let handler = this._pageStyleSyncHandlers.get(gBrowser.selectedBrowser.permanentKey);
> +    let data = this._pageStyleSheets.get(gBrowser.selectedBrowser.permanentKey);

Er, speaking of which... maybe we should actually use `browser` here!

::: browser/base/content/tab-content.js:435
(Diff revision 2)
> -
> +    addEventListener("pageshow", this);

Almost seems to make more sense to just 

addEventListener("pageshow", () => this.sendStyleSheetInfo());

rather than the level of indirection through handleEvent. Does that sound OK?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/21131/#review19105

> Almost seems to make more sense to just 
> 
> addEventListener("pageshow", () => this.sendStyleSheetInfo());
> 
> rather than the level of indirection through handleEvent. Does that sound OK?

Yep!

Comment 16

[:Gijs (he/him)]

Also, one thing I was considering for the allStylesheets/filteredStylesheets thing was doing the filtering on the content side so we don't send unnecessary stylesheets to the parent. IMO we could also shorten data: URIs to avoid sending manually constructed massive URIs (<link rel=stylesheet href="data:text/css,...">) between the processes. Dunno if that makes sense or if there's some reason not to do that that I missed (totally possible).

Comment 17

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/21131/#review19105

> So, I could be wrong, but it seems there are no callers of this inside the tree, and very few add-on callers. All of the ones relying on the forwarding behaviour from here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/3d7532ce81ac571962abc3b99582fe7f5d685192/browser/base/content/browser.js#5938
> 
> look like they expect they can pass a content window. Clearly that hasn't been possble for a while now, and I think e10s makes it impossible to do so sensibly.
> 
> The other issue with it that now this will return an empty array until after pageshow. This broke a test, and I imagine it might break usecases from add-ons, especially if any of them are doing any kind of caching.
> 
> And of course, we now no longer have CPOWs for the stylesheet objects, but just wrappers with a fraction of the info.
> 
> If this method is really unused inside the tree, I'd like to use the opportunity awarded by us breaking compat anyway, to remove it and its forwarding stuff (see above), and create a differently-named function that takes a browser (default to gBrowser.selectedBrowser), calls `_getStyleSheetInfo` and returns its `filteredStyleSheets` property. That should serve add-ons' needs (that or they can write their own content/frame script).
> 
> Does that sound sensible?
> 
> 
> Huh, reading more here, it even looks as if allStyleSheets is only used by this thing. Can we add the href to the list of properties for filteredStyleSheets and then kill off allStyleSheets altogether? Did I miss a use for allStyleSheets?

Alright, I've removed getAllStyleSheets, and added a new method to gPageStyleMenu called getBrowserStyleSheets which takes a browser (or defaults to the selected browser if the argument is omitted), and returns the filtered set (which now includes the stylesheet URL). Note that if the stylesheet URL is a data URL, I just return null for the URL. I considered choosing some fixed length that we'd allow to go through, and then I just said the hell with it.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669254 [details]
MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

The content process sends up the stylesheet info after the pageshow event,
so we should wait for that instead of load.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

https://reviewboard.mozilla.org/r/21131/#review19317

Nice, thanks!

::: browser/base/content/tab-content.js:470
(Diff revision 3)
> +    this.sendStyleSheetInfo();

I'm guessing this is here to ensure the parent-side info gets updated to be correct after something changes (we switch to a different style or disable author styles, or something?). Do we have a test for this behaviour? If not, can you add one?

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669254 [details]
MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

The content process sends up the stylesheet info after the pageshow event,
so we should wait for that instead of load.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669140 [details]
MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

Instead of using a CPOW to synchronously grab the stylesheet information
from the currently loaded tab, each tab now sends up the stylesheet
information once they've finished loading for the parent to cache.

Unfortunately, the cache will not be invalidated if the stylesheets on
a page are dynamically altered with script.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669254 [details]
MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

The content process sends up the stylesheet info after the pageshow event,
so we should wait for that instead of load.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1141041 - Add test to ensure that updating the Page Style menu works. r?Gijs

Bug 1141041 - Add test to ensure that updating the Page Style menu works. r?Gijs

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8671611 [details]
MozReview Request: Bug 1141041 - Add test to ensure that updating the Page Style menu works. r?Gijs

https://reviewboard.mozilla.org/r/21617/#review19471

::: browser/base/content/test/general/browser_page_style_menu_update.js:30
(Diff revision 1)
> + * selected Page Style after a newone has been selected.

s/newone/new one/

Comment 28

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/588505e93ed09b6260424316b2f7001bb4b85d59
Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/2e89754057bc81a916239a3d6342993bf453be1e
Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/5916ad1372e7c564aa7fbf67ccb895bc4953b6eb
Bug 1141041 - Add test to ensure that updating the Page Style menu works. r=jaws

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/588505e93ed0
https://hg.mozilla.org/mozilla-central/rev/2e89754057bc
https://hg.mozilla.org/mozilla-central/rev/5916ad1372e7

Comment 30

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 39.0a1 (Build ID: 20150309030224) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 44.0a1 (Build ID: 20151021065025)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

Comment 31

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug on Nightly 39.0a1 (2015-03-09) (Build ID: 20150309030224) in Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 44.0a1 (2015-10-25)

Build ID: 20151025030428
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

As it is also verified on Windows (Comment 30), Marking it as verified!

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1141041.html