Enabled in e10s -> new intermittent! Mike, any ideas?

https://brasstacks.mozilla.com/orangefactor/?display=Bug&tree=trunk&startday=2016-03-09&endday=2016-03-16&bugid=1257017 looks like this happens mostly on e10s but not only.

Grmbl. No clue, but I'll own it for starters.

So I'm seeing that something's wrong with the new `withSnippetsMap`; it sometimes doesn't resolve... I think.

My work is documented in the try runs in comment 5 up until comment 10, which is the run I'm very pleased with. First thing I noticed was that the intermittents were all 'hanging' in `withSnippetsMap()`, be it at different tasks, but still. Specifically, after adding more logging, I found that `onLocationChange` was not fired reliably in the WebProgressListener I added in the frame script, so I tried using the global progress listener provided on `gBrowser` and that worked much better. After this first big change I noticed that there were still intermittents left, but now 'hanging' at different tasks each time - but not the ones that use `withSnippetsMap`! Since they appeared - in the try runs I pushed - only in the search engine test ones I decided to cleanup inconsistent nsSearchService cleanup code and abolish the use of `registerCleanupFunction`. That was a good change but it didn't resolve the problem entirely. Doing `requestLongerTimeout(4)`, in the end, solved the remainder and I couldn't trigger an intermittent failure since.

Comment on attachment 8732114 [details] [diff] [review] Patch v1: use the global browser progress listener in 'withSnippetsMap' and request a longer timeout Review of attachment 8732114 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_aboutHome.js @@ +1,5 @@ > /* Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ > */ > > +requestLongerTimeout(4); heh, this indicates we need to largely split this test inot at least 3, and stop adding to it. Especially all the search tests that have been added here, it was not really the scope of this test to test the search field... :( Btw, nothing to do here, I'm just complaining loudly about past wrong decisions. Do we have a bug to do the splitting already? Ideally every time we add a requestLongerTimeout, we should have a bug to handle it (I know, this was here already, but still). @@ +571,5 @@ > setupFnSource = setupFn.toSource(); > } > > yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank" }, function* (browser) { > + let promises = []; unused? @@ +624,5 @@ > + onLocationChange: () => { > + gBrowser.removeProgressListener(wpl); > + // Phase 2: retrieving the snippets map is the next promise on our agenda. > + promise = promiseAfterLocationChange(); > + resolve(); why not just promiseAfterLocationChange.then(resolve) and later only yield once on promise? @@ +628,5 @@ > + resolve(); > + }, > + onProgressChange: () => {}, > + onStatusChange: () => {}, > + onSecurityChange: () => {} nit: in general the shorthand version, like "onProgressChange() {}", is more compact and less noisy than the arrow version

(In reply to Marco Bonardo [::mak] from comment #13) > Do we have a bug to do the splitting already? Ideally every time we add a > requestLongerTimeout, we should have a bug to handle it (I know, this was > here already, but still). I'll file a bug! Thanks for the review!

Patch for checkin.