1048618 - [e10s] make keyword-uri-fixup notification and associated chrome JS code work properly on e10s

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Named xxxx of local computer and running httpd server on the computer
2. Type xxxx in locationbar on e10s browser
   ---- googled xxxx , and the notification for localhosts does not appear --- bug

3. Type xxxx in locationbar on non-e10s browser
  ---- googled xxxx , and the notification for localhosts appear as expected

Comment 1

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

Looks like bug 693808's API should maybe use the message manager rather than the observer service?

Though if this was all on the chrome side it wouldn't be a problem - can't nsIURIFixupInfo reference the chrome-side docShell in question directly rather than making the UI jump through documentLoader/docshell.document/getBrowserForDocument hoops?

Comment 2

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #1)
> Looks like bug 693808's API should maybe use the message manager rather than
> the observer service?

I don't think this is possible - it's passing around XPCOM objects, which AIUI aren't serializable and therefore can't be passed through the message manager... Do I misunderstand the message manager?


> Though if this was all on the chrome side it wouldn't be a problem - can't
> nsIURIFixupInfo reference the chrome-side docShell in question directly
> rather than making the UI jump through
> documentLoader/docshell.document/getBrowserForDocument hoops?

I'm not sure the content-side docshell (which sends the notification) has this information in the e10s case. Does it?

Comment 3

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

(In reply to :Gijs Kruitbosch from comment #2)
> I don't think this is possible - it's passing around XPCOM objects, which
> AIUI aren't serializable and therefore can't be passed through the message
> manager... Do I misunderstand the message manager?

Anything can be serialized if you try hard enough! I assume you're referring to nsIURIFixupInfo? It's just some URIs, strings, booleans, and the "consumer" reference which could probably be just a window ID? Since it was recently introduced I assume we have some wiggle room to redesign the API.

> I'm not sure the content-side docshell (which sends the notification) has
> this information in the e10s case. Does it?

I don't know the best way to do it here exactly, associating a content dochsell to its chrome <browser> is doable in various ways.

Comment 4

[Dave Townsend [:mossop]]

What is the main code that is the problem here? I can't find a nsURLFixup anywhere.

Comment 5

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #4)
> What is the main code that is the problem here? I can't find a nsURLFixup
> anywhere.

The interface is nsIURIFixup, the implementation is nsDefaultURIFixup. The issue here is that docshell calls into it and sends out an observer notification which we listen for in browser.js. Presumably, the docshell sending this notification lives in content, while browser.js is in chrome.

https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4455

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#671

as noted in earlier comments, the API might need tweaking for this to be serializable and therefore something we can pass through a message manager.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch

Pretty simple, the only thing not serialisable is the docshell that generated the notification and since the message manager gives us the browser element that sent the message directly we don't need that.

This makes the test pass in e10s mode with some modification to use a progress listener in the child process since stopping the load isn't possible from the main process.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8482974 [details] [diff] [review]
patch

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

Generally, this looks OK. I've made some suggestions below. With those incorporated or refuted, r=me!

::: browser/base/content/browser.js
@@ +672,5 @@
>    }
>  };
>  
> +function gKeywordURIFixup({ target: browser, data: fixupInfo }) {
> +  let deserializeURI = (spec) => spec ? Services.io.newURI(spec, null, null) : null;

nit: use makeURI()

::: browser/base/content/content.js
@@ +643,5 @@
> +    return;
> +
> +  const URI_FIELDS = ["preferredURI", "fixedURI"]
> +  const FIELDS = ["fixupUsedKeyword", "fixupChangedProtocol",
> +                  "fixupCreatedAlternateURI", "originalInput"];

This worries me a little bit. Now we need to keep track of the fieldnames here if we want to change the interface, or the frontend code will break.

Why not something like:

let data = {};
for (let f of Object.keys(fixupInfo)) {
  if (typeof fixupInfo[f] != "function") {
    if (fixupInfo[f] && fixupInfo[f] instanceof Ci.nsIURI) {
      data[f] = fixupInfo[f].spec;
    } else if (!fixupInfo[f] || !fixupInfo[f].QueryInterface) {
      data[f] = fixupInfo[f];
    }
  }
}

Granted, it looks a little bit hacky, but it'll deal with new string/bool/int/uri fields without issues (I've not tested it, but the idea should be clear).

::: browser/base/content/test/general/head.js
@@ +404,5 @@
> +      onStateChange: function (webProgress, req, flags, status) {
> +        dump("waitForDocLoadAndStopIt: onStateChange " + flags.toString(16) + ": " + req.name + "\n");
> +        let docStart = Ci.nsIWebProgressListener.STATE_IS_DOCUMENT |
> +                       Ci.nsIWebProgressListener.STATE_START;
> +        if (((flags & docStart) == docStart) && webProgress.isTopLevel) {

Nit: Just flags & docStart should be enough here, as before...

@@ +425,5 @@
> +      resolve();
> +    }
> +
> +    let mm = aBrowser.messageManager;
> +    mm.loadFrameScript("data:,(" + content_script.toString() + ")();", true);

Nit: toSource() takes care of the () for you.

Comment 8

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #7)
> ::: browser/base/content/test/general/head.js
> @@ +404,5 @@
> > +      onStateChange: function (webProgress, req, flags, status) {
> > +        dump("waitForDocLoadAndStopIt: onStateChange " + flags.toString(16) + ": " + req.name + "\n");
> > +        let docStart = Ci.nsIWebProgressListener.STATE_IS_DOCUMENT |
> > +                       Ci.nsIWebProgressListener.STATE_START;
> > +        if (((flags & docStart) == docStart) && webProgress.isTopLevel) {
> 
> Nit: Just flags & docStart should be enough here, as before...

Unfortunately not. The tabbrowser webprogresslistener events are getting filtered somehow it seems. When we use the raw webprogress this trips on the event STATE_IS_DOCUMENT + STATE_STOP for about:blank.

> @@ +425,5 @@
> > +      resolve();
> > +    }
> > +
> > +    let mm = aBrowser.messageManager;
> > +    mm.loadFrameScript("data:,(" + content_script.toString() + ")();", true);
> 
> Nit: toSource() takes care of the () for you.

Not sure why but that doesn't seem to work in the actual test, even though it does in a quick check in a webpage.

Comment 9

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/4650bedd7691

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4650bedd7691

Comment 11

[:Gijs (he/him)]

I ran into this code again today while working on another bug... why do we remove the observer notification when an unload notification fires? That seems wrong... that's going to be a content-generated unload event, and the script won't get re-evaluated (to reinsert the keyword observer) when a new page loads, right?

Comment 12

[:Gijs (he/him)]

ie: http://hg.mozilla.org/mozilla-central/rev/4650bedd7691#l2.35 doesn't make sense to me.

Comment 13

[:Gijs (he/him)]

Hm, debugging this shows unload only fires when the content process goes away. That still doesn't make sense to me, but at least there isn't an actual issue here, so clearing needinfo...

Comment 14

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #13)
> Hm, debugging this shows unload only fires when the content process goes
> away. That still doesn't make sense to me, but at least there isn't an
> actual issue here, so clearing needinfo...

unload events don't bubble and that is an event listener that only captures bubbling events so it won't see them from content, only events dispatched directly to the event target, the frameloader in this case.