Intermittent toolkit/components/places/tests/browser/browser_visituri_privatebrowsing_perwindowpb.js | This test contains no passes, no fails and no todos. Maybe it threw a silent exception? Make sure you use waitForExplicitFinish() if you need it. -

RESOLVED FIXED in Firefox 55

Status

()

Product:
Toolkit
Component:
Places
Type:
defect
Importance:
P3
normal
Status:
RESOLVED FIXED
Reported:
3 years ago
Modified:
11 months ago

People

(Reporter: intermittent-bug-filer, Assigned: standard8)

Tracking

({intermittent-failure})

Version:
unspecified
Target:
mozilla56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [stockwell fixed])

Attachments

(1 attachment)

Bug 1292426 - Rewrite browser_visituri_privatebrowsing_perwindowpb.js to use modern async facilities and ensure we await on the final check to avoid intermittents.
59 bytes, text/x-review-board-request
mak
: review+
Details
Comment hidden (Intermittent Failures Robot)

Comment 2

•
3 years ago 
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Whiteboard: [stockwell unknown]
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 14

•
2 years ago 
Just happened to spot this. The test needs a bit of a rewrite - it registers an observer to check the results after a load, but doesn't actually wait on that observation being received. There is a wait for the page being loaded, which is probably why it passes most of the time, but I suspect those notifications can get out of sync.
Assignee: nobody → standard8

Comment 16

•
2 years ago
mozreview-review 
Comment on attachment 8887005 [details]
Bug 1292426 - Rewrite browser_visituri_privatebrowsing_perwindowpb.js to use modern async facilities and ensure we await on the final check to avoid intermittents.

https://reviewboard.mozilla.org/r/157764/#review162868

::: toolkit/components/places/tests/browser/browser_visituri_privatebrowsing_perwindowpb.js:24
(Diff revision 1)
> -          }
>  
>            // The expected visit should be the finalURL because private mode
>            // should not register a visit with the initialURL.
> -          uri = aSubject.QueryInterface(Ci.nsIURI);
> -          is(uri.spec, finalURL, "Check received expected visit");
> +          let uri = subject.QueryInterface(Ci.nsIURI);
> +          Assert.equal(uri.spec, finalURL, "Check received expected visit");

let's resolve to the uri.spec, and do the comparison in add_task? (see the other comment below).

::: toolkit/components/places/tests/browser/browser_visituri_privatebrowsing_perwindowpb.js:39
(Diff revision 1)
> -      aWin.close();
> +    await PlacesTestUtils.clearHistory()
> -    });
> +  });
> -  });
> +});
>  
> -  // test first when on private mode
> -  testOnWindow({private: true}, function(aWin) {
> +// Note: private browsing test must be run before the normal window one. See
> +// details in testLoadInWindow

I cannot find any details there, not in form of a comment at least.
Maybe the comment here should be more complete, something like:

"The private window test must be the first one to run, since we'll listen to the first uri-visit-saved notification, and we expect this test to not fire any, so we'll just find the non-private window test notification."

The misunderstanding I can imagine here is that initialUrl redirects to finalUrl, so potentially the first test may have a visit-saved notification for finalUrl... it's just that we only care about the first one.

::: toolkit/components/places/tests/browser/browser_visituri_privatebrowsing_perwindowpb.js:60
(Diff revision 1)
> +  let loadedPromise = BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +  await BrowserTestUtils.loadURI(win.gBrowser.selectedBrowser, url);
> +  await loadedPromise;
> +
> +  if (!options.private) {
> +    await visitSavedPromise;

move this to the second add_task? it sounds clearer like that, with the nicer comment. And then we can compare the urls there, that would give a better stack in case of errors.
Attachment #8887005 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 18

•
2 years ago 
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86ee390b7596
Rewrite browser_visituri_privatebrowsing_perwindowpb.js to use modern async facilities and ensure we await on the final check to avoid intermittents. r=mak

Comment 19

•
2 years ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/86ee390b7596
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 20

•
2 years ago
bugherderuplift 
https://hg.mozilla.org/releases/mozilla-beta/rev/eb96c581aa62
status-firefox55: --- → fixed
Flags: in-testsuite+
Whiteboard: [stockwell unknown] → [stockwell fixed]
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
You need to log in before you can comment on or make changes to this bug.