961529 - [e10s] Run DOMLinkHandler code in content script

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The DOMLinkHandler code handles favicons, RSS feeds, and browser search additions based on <link rel="whatever"> elements in the page. We need to handle the event in a content script and then notify the parent.

This is why favicons sometimes don't show up in e10s.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: utils-jsm

The current code calls isValidFeed, which calls urlSecurityCheck. These functions aren't available in content scripts. To solve this, I moved some things into JSMs, which I think is a lot better than what we have now with these overlays.

This first patch moves urlSecurityCheck and a few other convenience functions into a new Utils.jsm.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: feeds-jsm

This patch moves isValidFeed into Feeds.jsm.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: content-link-handler

This patch moves DOMLinkHandler into a content script. Since I know you don't want to make content.js too big, I actually put the code in a new JSM, ContentLinkHandler.jsm.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: content-link-handler v2

Tryserver revealed two other places that call getLinkIconURI.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

"Utils.jsm" is a little too generic, I think. I would prefer "BrowserUtils" (ala bug 655747).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just realized that moving a security check to a content process is a bad idea. I guess to make this work right I'll need to figure out exactly what urlSecurityCheck does.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8364540 [details] [diff] [review]
content-link-handler v2

We decided that doing the security check in the parent doesn't offer any additional security since it still would be trusting the child to tell it the right principals.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8363376 [details] [diff] [review]
content-link-handler

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

::: browser/modules/ContentLinkHandler.jsm
@@ +17,5 @@
> +  "resource:///modules/Feeds.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> +  "resource://gre/modules/Utils.jsm");
> +
> +let ContentLinkHandler = {

this.ContentLinkHandler =

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8363374 [details] [diff] [review]
utils-jsm

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

All nits, except for the isContentFrame function issue

::: toolkit/content/contentAreaUtils.js
@@ +37,5 @@
>  
> +// Expose everything in Utils.
> +Components.utils.import("resource://gre/modules/Utils.jsm");
> +for (let func in Utils)
> +  this[func] = Utils[func];

while this is clean, it makes it hard to grep the code and understand how it's getting called. So I prefer if we do the slightly verbose:

function urlSecurityCheck(aURL, aPrincipal, aFlags) {
 return Utils.urlSecurityCheck(aURL, aPrincipal, aFlags);
}

With this we can also add newer functions to Utils and they won't be unknowingly exposed here by mistake

::: toolkit/modules/Utils.jsm
@@ +1,3 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.

(very nitpicky, but most .jsms I can find use /* */ for the license, so let's keep a consistent style)

@@ +9,5 @@
> +const {interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let Utils = {

this.Utils =

@@ +55,5 @@
> +  isContentFrame: function(aFocusedWindow) {
> +    if (!aFocusedWindow)
> +      return false;
> +
> +    return (aFocusedWindow.top == window.content);

hmm, this will not work in the context of the jsm anymore.  Can just function be kept in contentAreaUtils.js?  Otherwise we'll have to pass the window as a parameter..

Or if you can think of a way to fix this in an e10s compat way, even better :)

::: toolkit/modules/moz.build
@@ +43,5 @@
>      'Sqlite.jsm',
>      'Task.jsm',
>      'TelemetryTimestamps.jsm',
>      'Timer.jsm',
> +    'Utils.jsm',

Following Gavin's suggestion, let's name this something more specific. But since this is in toolkit, perhaps ContentAreaUtils.jsm (very creative..) is better than BrowserUtils.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8363375 [details] [diff] [review]
feeds-jsm

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

::: browser/modules/Feeds.jsm
@@ +8,5 @@
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> +                                  "resource://gre/modules/Utils.jsm");

(reminder to update this path with the new name from part 1)

@@ +12,5 @@
> +                                  "resource://gre/modules/Utils.jsm");
> +
> +const Ci = Components.interfaces;
> +
> +let Feeds = {

this.Feeds =

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8364540 [details] [diff] [review]
content-link-handler v2

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

(ops, wrong attachment before)

::: browser/modules/ContentLinkHandler.jsm
@@ +17,5 @@
> +  "resource:///modules/Feeds.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> +  "resource://gre/modules/Utils.jsm");
> +
> +let ContentLinkHandler = {

this.ContentLinkHandler =

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: utils-jsm v2

I just took isContentFrame out of the module. Thanks for noticing that.

I named the module BrowserUtils because ContentAreaUtils is already used as a sort-of module name in other places (including contentAreaUtils.js), and I didn't want to add confusion.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f413d6ff704
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/639da97e1703
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ea08c51560

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/0f413d6ff704
https://hg.mozilla.org/mozilla-central/rev/639da97e1703
https://hg.mozilla.org/mozilla-central/rev/e6ea08c51560

Comment 17

[:Gijs (he/him)]

There are still tests disabled on e10s with a note pointing to the dupe (bug 918663). Should those be re-enabled?

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We're not running browser-chrome tests in e10s at all. I guess it's fine to remove the annotation, but it won't do anything right now.