Re-enable browser_progress_keyword_search_handling.js for Fission. (Filing this ahead of landing the regressing patch in order to be able to refer to this bug's number.)

Tracking for Fission mochitests (M4.1)

nsFocusManager::SetFocus bug 1556627 regressed and disabled this test for Fission, so we now need to fix and re-enable it.

Bug 1614268 suggests that mActiveWindow doesn't get nulled out soon enough.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56ff93f54fb475263bdb9dd1bb0bc768d2aeacc9

Now says

TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_progress_keyword_search_handling.js | Should be showing stop

instead of leaking an about:neterror.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=028fbee53ea1433785791cda9817b4dd3f7cbdc0

Henri, what are the next steps for re-enabling this test for Fission?

The Try run from last month (comment 6) has the same failure reason as from two months ago (comment 5):

TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_progress_keyword_search_handling.js | Should be showing stop -

:mccr8 is very good at debugging Fission memory leaks. I could ask him to look at this test.

The Fission team hopes to roll out Fission to some Nightly users in Q3, so it would be good if all tests are passing and enabled for Fission by the end of Q2.

New try run with the patch for bug 1634363:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7dc6edb966860d49fb775552d8071681b1eb08

(In reply to Chris Peterson [:cpeterson] from comment #7)

:mccr8 is very good at debugging Fission memory leaks. I could ask him to look at this test.

AFAICT, per comment 5, this no longer leaks a window until shutdown. Gijs, since you wrote the test, can you interpret the probable cause of https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/browser/base/content/test/tabs/browser_progress_keyword_search_handling.js#32 failing?

The test puts a single word in the URL bar and hits enter. Because the single word shouldn't load any webpages, docshell then redirects to a search page ( only if the relevant pref is true, https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/browser/base/content/test/tabs/browser_progress_keyword_search_handling.js#11 - otherwise, the url bar will proactively assume that typing a single word will cause a search, rather than trying to do a dns lookup first).

The test then checks that while the search is loading, the stop/refresh button shows a stop button. Then it stops the load (using BrowserTestUtils.waitForDocLoadAndStopIt, and checks that we show a refresh button again.

The test failing shows that, before we stop the load, the stop button is not being displayed. That probably means that either we never switched to show the stop button, or something else already switched us back to a refresh button while the load was still ongoing. Neither of those should happen.

The failure is happening in the parent process document channel case, and the test passes in the non-ppdc case, based on the log. So my initial suspicion would be that something changed about what progress listener notifications get fired. However, the BTU code inside waitForDocLoadAndStopIt already filters those notifications, so I'm not really sure how that's possible.

Does that help?

(reading bug 1591183 would also help provide more context for the case the test is supposed to deal with.)

Oh, and if this fails reliably, bisecting when it started failing using artifact builds may also help narrow down what broke, because I'm fairly sure we changed the order / set of events the docshell sends in the ppdc case again since the test was created...

Thanks. It does fail reliably, but only in Fission. Also, your description does not implicate an obvious dependency on Fission focus code (it was disabled due to bug 1556627 causing a window leak until shutdown, which was focus-related but has already been fixed). Is it possible that the documentchannel event order changed for Fission only while this test has been disabled for Fission?

(In reply to Henri Sivonen (:hsivonen) from comment #14)

Is it possible that the documentchannel event order changed for Fission only while this test has been disabled for Fission?

Yes. Pretty sure ppdc was not enabled for all protocols / everywhere 3-4 months ago; that happened much more recently (bug 1632098 an deps / other work in that area). IIRC there were more event ordering changes and test issues as a result; :jya struggled with a bunch of those. This one may have been missed because the test was disabled.

jya, could you, please, take a look at the way this test fails if enabled for Fission?

Jean-Yves says this test is waiting for HTTP preconnect event, but DocumentChannel doesn't support preconnect event yet.

Assigning to Jean-Yves because he (or Matt Woodrow) will add DocumentChannel preconnect support and fix this test.

https://hg.mozilla.org/mozilla-central/rev/ba4a9fee6732

When rebasing my code from bug 1640019 on top of this change, I noticed that mRequestCount is not being persisted when destroying and re-creating the TabProgressListener in swapBrowserDocShells: https://hg.mozilla.org/mozilla-central/annotate/tip/browser/base/content/tabbrowser.js#l4148

Is this intentional, or should we be preserving this value there as well?

I would guess that it should be preserved there too.

You'd have to do tab dragging, while a page is loading, and then for it to fail with a bad uri error for it to matter.

Maybe we can just fix the underlying bug here instead, I'm going to think about it.

So nsDocLoader::doStopDocumentLoad fires two slightly-different state changes - https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/uriloader/base/nsDocLoader.cpp#934

The first of these is handled by nsDocShell, and it calls EndPageLoad, which can synchronously trigger another load (usually a background load of an error page, but with uri fixup it can trigger a normal load).

The latter of these two is the one that gets through the nsBrowserStatusFilter, and gets sent to the parent process as the 'load has finished' marker.

Unfortunately, if we did start a new load due to uri fixup, that will have synchronously sent yet another state change for the start of the new load.

This results in the frontend seeing the start of the new load before the stop of the previous one, and then it has to do workarounds with mRequestCount to try untangle this.

Does anyone have thoughts on the best way to fix this?

We could make the load for urifixup happen asynchronously, but we'd have to handle something else starting a load in the meantime, and we still have the possibility of other progress listeners doing the same thing.