Closed
Bug 1139588
Opened 10 years ago
Closed 10 years ago
Intermittent browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js | Test timed out | Found a browser window
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
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)
8.20 KB,
text/plain
|
Details | |
3.33 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
See comment 1 and the attached log for details.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Iteration: --- → 39.1 - 9 Mar
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 33•10 years ago
|
||
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 34•10 years ago
|
||
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+
Comment 35•10 years ago
|
||
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?
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 37•10 years ago
|
||
(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 38•10 years ago
|
||
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-
Assignee | ||
Comment 39•10 years ago
|
||
Maintain a set of listeners rather than just a single one.
Attachment #8573514 -
Attachment is obsolete: true
Attachment #8573864 -
Flags: review?(gijskruitbosch+bugs)
Updated•10 years ago
|
Attachment #8573864 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 40•10 years ago
|
||
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 44•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Reporter | ||
Updated•10 years ago
|
status-firefox37:
--- → unaffected
status-firefox38:
--- → unaffected
status-firefox-esr31:
--- → unaffected
Reporter | ||
Comment 45•10 years ago
|
||
Actually, this is a more-generic fix. Let's uplift this as far as we can.
Reporter | ||
Comment 46•10 years ago
|
||
Reporter | ||
Comment 47•10 years ago
|
||
Comment hidden (Legacy TBPL/Treeherder Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•