All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: alice0775, Assigned: mossop)

Tracking

(Depends on: 1 bug)

34 Branch
mozilla35
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
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

4 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
Flags: needinfo?(gavin.sharp)
(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.
Flags: needinfo?(gavin.sharp)

Updated

4 years ago
Flags: firefox-backlog?
Summary: [e10s] the notification for localhosts does not appear in e10s → [e10s] the notification for localhosts does not appear in e10s

Updated

4 years ago
tracking-e10s: --- → ?
Assignee: nobody → mrbkap
Blocks: 997462
tracking-e10s: ? → +
Summary: [e10s] the notification for localhosts does not appear in e10s → [e10s] nsURLFixup needs to be e10s aware
Component: Location Bar → Document Navigation
Product: Firefox → Core
Flags: firefox-backlog?
(Assignee)

Comment 4

4 years ago
What is the main code that is the problem here? I can't find a nsURLFixup anywhere.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

4 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Updated

4 years ago
Summary: [e10s] nsURLFixup needs to be e10s aware → [e10s] make keyword-uri-fixup notification and associated chrome JS code work properly on e10s
(Assignee)

Updated

4 years ago
Assignee: mrbkap → dtownsend+bugmail
(Assignee)

Comment 6

4 years ago
Created attachment 8482974 [details] [diff] [review]
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.
Attachment #8482974 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED

Comment 7

4 years ago
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.
Attachment #8482974 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 8

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/4650bedd7691
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 11

4 years ago
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?
Flags: needinfo?(dtownsend)

Comment 13

4 years ago
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...
Flags: needinfo?(dtownsend)
(Assignee)

Comment 14

4 years ago
(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.
Depends on: 1128937
You need to log in before you can comment on or make changes to this bug.