Closed Bug 1041297 Opened 10 years ago Closed 10 years ago

browser_tab_dragdrop2.js | uncaught exception - TypeError: doc.defaultView.test_panels is not a function

Categories

(Firefox :: Tabbed Browser, defect)

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

Tracking

()

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

People

(Reporter: miker, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

0001-Bug-1041297-Rewrite-browser_tab_dragdrop2.js-to-get-.patch, v2
5.71 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
No description provided.
Tim, this is failing pretty frequent on m-e10s. Do you have time to look into this?
tracking-e10s: --- → ?
Component: General → Tabbed Browser
Flags: needinfo?(ttaubert)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #216) > Tim, this is failing pretty frequent on m-e10s. Do you have time to look > into this? Wow, this test is terrible. The setTimeout(1000) here is obviously a problem. When the browser is slow (i.e. ASAN builds) then that timeout isn't enough and we run into said error. Easily reproducible by reducing the timeout to 10ms on an OS X opt build at least. I'll look into it.
Flags: needinfo?(ttaubert)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Rewrote the test completely to use all the new shiny things we have. The popup tests in the separate file run asynchronously so I added a "done" variable to simply check whether that has finished. We can just access browser.contentWindow as the test page is XUL and that won't ever be remote. WFM w/ and w/o e10s locally. I'll push to try.
Attachment #8570054 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8570054 [details] [diff] [review] 0001-Bug-1041297-Rewrite-browser_tab_dragdrop2.js-to-get-.patch Review of attachment 8570054 [details] [diff] [review]: ----------------------------------------------------------------- Shudder. Much better, but... ::: browser/base/content/test/general/browser_tab_dragdrop2.js @@ +36,5 @@ > + > +function promiseNewWindow(url) { > + let args = "chrome,all,dialog=no"; > + let win = window.openDialog(getBrowserURL(), "_blank", args, url); > + return promiseDelayedStartupFinished(win).then(() => win); Why can't we just make the promiseDelayedStartupFinished thing have a promise resolve with the window? This looks a bit weird. You also only use this function once... @@ +46,5 @@ > + }); > +} > + > +function promiseTestsDone(browser) { > + return promiseWaitForCondition(() => { Hm, so, maybe you know the answer to this... waitForCondition uses setTimeout. Why don't these tests need the flaky timeout thing that Ehsan added? And, if we want to avoid these timeouts, and we're modifying the .xul file anyway, can't we just fire a custom event from the window and listen for that event?
Attachment #8570054 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #228) > Hm, so, maybe you know the answer to this... waitForCondition uses > setTimeout. Why don't these tests need the flaky timeout thing that Ehsan > added? Good question. Haven't this restriction even once so far, I wonder how that's implemented. > And, if we want to avoid these timeouts, and we're modifying the .xul file > anyway, can't we just fire a custom event from the window and listen for > that event? Yeah, good idea. Done.
Attachment #8570054 - Attachment is obsolete: true
Attachment #8571287 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8571287 [details] [diff] [review] 0001-Bug-1041297-Rewrite-browser_tab_dragdrop2.js-to-get-.patch, v2 Review of attachment 8571287 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_tab_dragdrop2.js @@ +21,5 @@ > + > + // Tear off the original tab. > + let browser = win.gBrowser.selectedBrowser; > + let tabClosed = promiseWaitForEvent(browser, "pagehide", true); > + let win2 = win.gBrowser.replaceTabWithWindow(win.gBrowser.tabs[0]); I wonder if we should test here that the tests don't run again (ie the page is not reloaded) ? @@ +26,5 @@ > + yield Promise.all([tabClosed, promiseDelayedStartupFinished(win2)]); > + > + // Run tests once again. > + win2.content.test_panels(); > + yield promiseTestsDone(win2); This ordering makes me nervous in case we ever make the tests run synchronously. I'd define the promise before the event could be fired, then run the test, then yield for the promise. Any reason not do that?
Attachment #8571287 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #256) > > + // Tear off the original tab. > > + let browser = win.gBrowser.selectedBrowser; > > + let tabClosed = promiseWaitForEvent(browser, "pagehide", true); > > + let win2 = win.gBrowser.replaceTabWithWindow(win.gBrowser.tabs[0]); > > I wonder if we should test here that the tests don't run again (ie the page > is not reloaded) ? Yeah thought about doing that too. Sure, why not. > > + // Run tests once again. > > + win2.content.test_panels(); > > + yield promiseTestsDone(win2); > > This ordering makes me nervous in case we ever make the tests run > synchronously. I'd define the promise before the event could be fired, then > run the test, then yield for the promise. Any reason not do that? Heh, had the same thought again. Might be a good idea to make it future-proof.
Blocks: e10s-tests
tracking-e10s: ? → +
(In reply to Wes Kocher (:KWierso) from comment #276) > Backed out in https://hg.mozilla.org/integration/fx-team/rev/9740c8bfd125 > for the spike in bug 1001656 it apparently caused. Sorry for that, currently trying to reproduce it in a Linux VM without much success.
Blocks: 1001656
Flags: needinfo?(ttaubert)
Pushed a new version to try that calls waitForFocus() before opening a popup: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0873230abf
(In reply to Tim Taubert [:ttaubert] from comment #292) > Pushed a new version to try that calls waitForFocus() before opening a popup: Yeah... so that includes a lot of failures. BUT, good news: I was stupid and forgot to qpush the actual patch. Let's try that again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0d5f1ff57bd
Landed again with the waitForFocus() change: https://hg.mozilla.org/integration/fx-team/rev/1af9f2bb563f
https://hg.mozilla.org/mozilla-central/rev/1af9f2bb563f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
This hit failures on beta that aen't worth trying to track down. Backed out. https://hg.mozilla.org/releases/mozilla-beta/rev/33176406bcfe
status-firefox37: fixed → wontfix
Blocks: 1140887
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: