Closed Bug 1257017 Opened 8 years ago Closed 8 years ago

Intermittent browser_aboutHome.js | Found a tab after previous test timed out: about:home -

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: KWierso, Assigned: mikedeboer)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Enabled in e10s -> new intermittent! Mike, any ideas?
Blocks: 1093153
Flags: needinfo?(mdeboer)
Grmbl. No clue, but I'll own it for starters.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
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.
Attachment #8732114 - Flags: review?(mak77)
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
Attachment #8732114 - Flags: review?(mak77) → review+
(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!
Blocks: 1258717
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61c6db0f7e1b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1260330
You need to log in before you can comment on or make changes to this bug.