Closed Bug 1141041 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
e10s m8+ ---
firefox44 --- verified

People

(Reporter: Kwan, Assigned: mconley)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(4 files)

+++ 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.
tracking-e10s: m8+ → ---
Whiteboard: [unsafe-cpow-usage]
Still around.
tracking-e10s: --- → ?
Whiteboard: [unsafe-cpow-usage]
Assignee: nobody → wmccloskey
Hoping I can steal this one too.
Assignee: wmccloskey → 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.
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.
The other option is to just stop supporting alternative stylesheets altogether, which would certainly be the simplest option.
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).
Keywords: addon-compat
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.
Attachment #8669140 - Attachment description: MozReview Request: Bug 1141041 - [WIP] Stop using CPOWs for the Page Style menu. → MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs)
Looks like I have some failing tests to figure out.
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.
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8669254 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8669254 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs)
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!
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).
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 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.
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8669254 - Attachment description: MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r?Gijs → MozReview Request: Bug 1141041 - Update browser_page_style_menu.js to wait for pageshow event. r=Gijs
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?
Attachment #8669140 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8669140 - Attachment description: MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r?Gijs → MozReview Request: Bug 1141041 - Stop using CPOWs for the Page Style menu. r=Gijs
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 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 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 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/
Attachment #8671611 - Flags: review?(jaws) → review+
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
QA Whiteboard: [bugday-20151021]
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!
Status: RESOLVED → VERIFIED
Depends on: 1228801
Depends on: 1271049
Depends on: 1390928
You need to log in before you can comment on or make changes to this bug.