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

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.

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

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

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

Depends on D81944

Depends on D81945

Depends on D81946

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

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?

(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".

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.

(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.

Depends on D81947

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