1649879 - Stop running URIFixup.jsm from the content process

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task)

Description

[Matt Woodrow (:mattwoodrow)]

We should be able to avoid invoking this from content process docshells, and then we won't need to load the .jsm

Comment 1

[Marco Bonardo [:mak]]

bug 1640132 may help removing the ipc calls, that only exist because the content process tries to fixup strings (urls) to searches, but that should only be done by the urlbar. Then there isn't much left that couldn't be moved around.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Fold GetFixupURIInfo into the calling code. r?kmag!

This should be a no-op change, just to make the next patch a small diff.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Don't create nsIURIFixupInfo in content process nsDocShellLoadState construction. r?kmag!

Rather than constructing an nsIURIFixupInfo from the IPC call return valuess, and then immediately querying the same data, this just use the results directly.

It also moves the firing of "keyword-uri-fixup" observers to the parent process side. As far as I can tell, the only consumer was URIFixupChild, which was also forwarding them to the parent process.

Depends on D81943

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Implement WebNavigationFlagsToFixupFlags in C++ so that we avoid needing to call into the URIFixup JS module. r?kmag!

This should be the exact same code, just avoiding needing to create the URIFixup service in order to run it.

Depends on D81944

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Remove URIFixupChild. r?kmag!

Depends on D81945

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Handle URIFixup that happens on a failed channel in DocumentLoadListener if available, rather than waiting for it to reach nsDocShell. r?kmag!

Depends on D81946

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13b19e14a332
Fold GetFixupURIInfo into the calling code. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6f905d33681f
Don't create nsIURIFixupInfo in content process nsDocShellLoadState construction. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6dc2e9474f43
Implement WebNavigationFlagsToFixupFlags in C++ so that we avoid needing to call into the URIFixup JS module. r=kmag
https://hg.mozilla.org/integration/autoland/rev/76ab8adad34b
Remove URIFixupChild. r=kmag
https://hg.mozilla.org/integration/autoland/rev/f9670eed4ac5
Handle URIFixup that happens on a failed channel in DocumentLoadListener if available, rather than waiting for it to reach nsDocShell. r=kmag

Comment 8

[Cristina Coroiu [:ccoroiu]]

Backed out 5 changesets (bug 1649879) for browser-chrome failures at browser/base/content/test/tabs/browser_progress_keyword_search_handling.js

Backout: https://hg.mozilla.org/integration/autoland/rev/c812a36a5646aee44b39bebbe862b63a746bcf69

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f9670eed4ac5382cf1c67115426645d10fb14c6c

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308661328&repo=autoland&lineNumber=18070

Comment 9

[Matt Woodrow (:mattwoodrow)]

Oh, looks like browser_progress_keyword_search_handling.js fails because we can now start a speculative load in the parent, before the docshell has had a chance to see it.

The test is starting a load of an invalid uri, and then using waitForDocLoadAndStopIt (which uses the underlying http channel events) detect when we attempt to load the fixed uri.

With speculative loading, and uri fixup running in the parent process, this can all happen quickly, while we're still in the process of notifying the docshell of the same load.

The test is checking that the stop/reload button is showing stop when the load attempts to connect, which it isn't (since the stop/reload button changes in response to web progress events, which come from the docshell).

Gijs, do you have any ideas how to fix this test, without breaking the intended coverage?

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)

Oh, looks like browser_progress_keyword_search_handling.js fails because we can now start a speculative load in the parent, before the docshell has had a chance to see it.

The test is starting a load of an invalid uri, and then using waitForDocLoadAndStopIt (which uses the underlying http channel events) detect when we attempt to load the fixed uri.

With speculative loading, and uri fixup running in the parent process, this can all happen quickly, while we're still in the process of notifying the docshell of the same load.

The test is checking that the stop/reload button is showing stop when the load attempts to connect, which it isn't (since the stop/reload button changes in response to web progress events, which come from the docshell).

Gijs, do you have any ideas how to fix this test, without breaking the intended coverage?

I'm really sorry but I'm not following the description. What are we loading speculatively (ie http://idontreallyexist or the search engine URL?), and why does the speculative load break the test? Does the speculative load hit the code in waitForDocLoadAndStopIt?

Does just turning off speculative loads (IIRC there's a pref) fix things?

We could also consider switching the test to registering a dummy search engine, then we wouldn't need to stop the load to avoid external connections - but we still need to assert that the button switches to "stop" and back to "refresh".

Comment 11

[Kris Maglione [:kmag]]

I think our best bet is to register a search engine that the mochitest server can handle, and change the test to use web progress events rather than HTTP events. That's closer to the behavior we actually want to test for, in any case, and should be much less fragile.

In the future, we really want the web progress events in the parent to be generated by DocumentLoadListener in the parent rather than DocShells in the child, which would also make this much less of an issue. But it's also nontrivial to implement.

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to :Gijs (he/him) from comment #10)

I'm really sorry but I'm not following the description. What are we loading speculatively (ie http://idontreallyexist or the search engine URL?), and why does the speculative load break the test? Does the speculative load hit the code in waitForDocLoadAndStopIt?

Strictly speaking, both of the URLs go through the speculative load, but it's the search engine URL that triggers a real http connection attempt and is indeed caught by waitForDocLoadAndStopIt (and would crash for attempting an external connection if it didn't).

Does just turning off speculative loads (IIRC there's a pref) fix things?

Yeah, that would work.

We could also consider switching the test to registering a dummy search engine, then we wouldn't need to stop the load to avoid external connections - but we still need to assert that the button switches to "stop" and back to "refresh".

I'm going to have a go at doing this.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1649879 - Use webprogress events instead of waitForDocLoadAndStopIt in browser_progress_keyword_search_handling.js. r?Gijs

Depends on D81947

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9820dc5ac04e
Fold GetFixupURIInfo into the calling code. r=kmag
https://hg.mozilla.org/integration/autoland/rev/280e5d62cd74
Don't create nsIURIFixupInfo in content process nsDocShellLoadState construction. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b0e4bfaf5370
Implement WebNavigationFlagsToFixupFlags in C++ so that we avoid needing to call into the URIFixup JS module. r=kmag
https://hg.mozilla.org/integration/autoland/rev/9ca98f13b6b8
Remove URIFixupChild. r=kmag
https://hg.mozilla.org/integration/autoland/rev/33bcefd78000
Handle URIFixup that happens on a failed channel in DocumentLoadListener if available, rather than waiting for it to reach nsDocShell. r=kmag
https://hg.mozilla.org/integration/autoland/rev/0042204c7d35
Use webprogress events instead of waitForDocLoadAndStopIt in browser_progress_keyword_search_handling.js. r=Gijs

Comment 15

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9820dc5ac04e
https://hg.mozilla.org/mozilla-central/rev/280e5d62cd74
https://hg.mozilla.org/mozilla-central/rev/b0e4bfaf5370
https://hg.mozilla.org/mozilla-central/rev/9ca98f13b6b8
https://hg.mozilla.org/mozilla-central/rev/33bcefd78000
https://hg.mozilla.org/mozilla-central/rev/0042204c7d35