Closed Bug 1139588 Opened 9 years ago Closed 9 years ago

Intermittent browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js | Test timed out | Found a browser window

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Flags:
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: ttaubert)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

test log
8.20 KB, text/plain
Details
0001-Bug-1139588-Fix-waitForDocLoadComplete-to-hold-onto-.patch, v2
3.33 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
Attached file test log — 
See comment 1 and the attached log for details.
I love weak references. Looks just like bug 1022403, a weak progress listener is GC'ed before we see the event we're waiting for. This time it's in general/head.js, so it might affect a few more intermittent oranges.
Weak refs can be such a footgun :/ I can reproduce that failure locally by calling .loadURI() off a timeout to increase chances the progress listener is GC'ed before the page is loaded. With that patch it doesn't fail anymore.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8573514 - Flags: review?(mconley)
Iteration: --- → 39.1 - 9 Mar
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8573514 [details] [diff] [review]
0001-Bug-1139588-Fix-waitForDocLoadComplete-to-hold-onto-.patch

Optimizing for timezones in the hope that Gijs has a few spare cycles :)

Looking at what tests use waitForDocLoadComplete(), these bugs *might* be solved as well:

bug 1101624, bug 1093660, bug 1137965, bug 1082678
Attachment #8573514 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8573514 [details] [diff] [review]
0001-Bug-1139588-Fix-waitForDocLoadComplete-to-hold-onto-.patch

Review of attachment 8573514 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but please see nit.

::: browser/base/content/test/general/head.js
@@ +414,5 @@
>   * @return promise
>   */
>  function waitForDocLoadComplete(aBrowser=gBrowser) {
> +  return new Promise(resolve => {
> +    this.progressListener = {

|this| is going to be the same as |window|, isn't it? If so, I think we should use a more unique variable name (say, _testDocLoadCompleteListener or whatever).
Attachment #8573514 - Flags: review?(mconley)
Attachment #8573514 - Flags: review?(gijskruitbosch+bugs)
Attachment #8573514 - Flags: review+
Also... wait, this is just confusing.

When is the progress listener ever removed from |this| ? And if we keep a reference like this, what's the use of making core code keep a weak ref?
(In reply to :Gijs Kruitbosch from comment #35)
> When is the progress listener ever removed from |this| ?

It's not until the end of the test. Or the next waitForDocLoadComplete() call, which is bad.

> And if we keep a
> reference like this, what's the use of making core code keep a weak ref?

I don't know why it was decided that the nsIWebProgress interface accepts only weak listeners. Same for nsISHistory. Maybe back then it was done to try and prevent leaks?
Comment on attachment 8573514 [details] [diff] [review]
0001-Bug-1139588-Fix-waitForDocLoadComplete-to-hold-onto-.patch

(In reply to Tim Taubert [:ttaubert] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #35)
> > When is the progress listener ever removed from |this| ?
> 
> It's not until the end of the test. Or the next waitForDocLoadComplete()
> call, which is bad.

Ah. Yeah. Um. Let's try again. :-\
Attachment #8573514 - Flags: review+ → review-
Maintain a set of listeners rather than just a single one.
Attachment #8573514 - Attachment is obsolete: true
Attachment #8573864 - Flags: review?(gijskruitbosch+bugs)
Attachment #8573864 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/6042c5275ad6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Actually, this is a more-generic fix. Let's uplift this as far as we can.
status-firefox37: unaffected → affected
status-firefox38: unaffected → affected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: