Closed Bug 1041297 Opened 10 years ago Closed 9 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.