Closed Bug 855763 Opened 7 years ago Closed 7 years ago

[Responsive Mode] Intermittent browser_responsiveuiaddcustompreset.js | Test timed out, | Found a tab after previous test timed out: data:text/html,foo, browser_Services.js | Services.prompt is an nsIPromptService

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: philor, Assigned: msucan)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #798775 +++

https://bugzilla.mozilla.org/show_bug.cgi?id=798775#c26
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=21188537&tree=Firefox
Rev3 WINNT 5.1 mozilla-central opt test mochitest-browser-chrome on 2013-03-27 20:33:14
slave: talos-r3-xp-038

20:51:05  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | Test timed out
20:51:06  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | Found a tab after previous test timed out: data:text/html,foo
20:56:29  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_Services.js | Services.prompt is an nsIPromptService

https://tbpl.mozilla.org/php/getParsedLog.php?id=21205732&tree=Firefox
WINNT 6.2 mozilla-central pgo test mochitest-browser-chrome on 2013-03-28 06:41:11 PDT for push 962f5293f87f
slave: t-w864-ix-024

06:55:36     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | number of presets didn't change after add preset and cancel
06:55:44     INFO -  1364478944916	Services.DataReporting.Policy	DEBUG	Next data submission is scheduled in the future: Fri Mar 29 2013 06:48:44 GMT-0700 (Pacific Standard Time)
06:56:05  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | Test timed out
(screenshot)
06:56:06     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | finished in 29989ms
06:56:06  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | Found a tab after previous test timed out: data:text/html,foo
06:56:06     INFO -  ReferenceError: info is not defined: restart@chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js:80
06:56:06     INFO -  @resource:///modules/devtools/EventEmitter.jsm:56
06:56:06     INFO -  EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100
06:56:06     INFO -  RUI_unload@resource://app/modules/devtools/responsivedesign.jsm:240
06:56:06     INFO -  @resource://app/modules/devtools/responsivedesign.jsm:264
06:56:06     INFO -  _beginRemoveTab@chrome://browser/content/tabbrowser.xml:1668
06:56:06     INFO -  removeTab@chrome://browser/content/tabbrowser.xml:1563
06:56:06     INFO -  Tester_waitForWindowsState@chrome://mochikit/content/browser-test.js:138
06:56:06     INFO -  Tester_nextTest@chrome://mochikit/content/browser-test.js:344
06:56:06     INFO -  @chrome://mochikit/content/browser-test.js:435
06:56:06     INFO -  TEST-INFO | checking window state
06:56:06     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js | Console message: [JavaScript Error: "ReferenceError: info is not defined: restart@chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js:80
06:56:06     INFO -  @resource:///modules/devtools/EventEmitter.jsm:56
06:56:06     INFO -  EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100
06:56:06     INFO -  RUI_unload@resource://app/modules/devtools/responsivedesign.jsm:240
06:56:06     INFO -  @resource://app/modules/devtools/responsivedesign.jsm:264
06:56:06     INFO -  _beginRemoveTab@chrome://browser/content/tabbrowser.xml:1668
06:56:06     INFO -  removeTab@chrome://browser/content/tabbrowser.xml:1563
06:56:06     INFO -  Tester_waitForWindowsState@chrome://mochikit/content/browser-test.js:138
06:56:06     INFO -  Tester_nextTest@chrome://mochikit/content/browser-test.js:344
06:56:06     INFO -  @chrome://mochikit/content/browser-test.js:435
06:56:06     INFO -  " {file: "resource:///modules/devtools/EventEmitter.jsm" line: 105}]
07:00:15  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_Services.js | Services.prompt is an nsIPromptService
Mihai, can you look into this please?
Flags: needinfo?(mihai.sucan)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #40)
> Mihai, can you look into this please?

I will look into this as soon as possible. I'll try this week or next week.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Flags: needinfo?(mihai.sucan)
Attached patch proposed fix (obsolete) — Splinter Review
I was able to reproduce intermittent failures on my system which meant it's easier for me to fix this. I made a number of changes:

- the test was slightly too eager to do things after responsive mode enable/disable. I added some executeSoons.

- there was at least one missing listener for responsive mode enable/disable.

- the key synthesizer keeps failing intermittently for me. I just switched that to doCommand(). This test is not about checking if xul:key elements work, or if the synthesizeKey() function works. We can assume that if the key.doCommand() method works the actual keyboard shortcut also works.

No more failures on my system. I'll land this in inbound once I get r+. Please let me know if I broke anything. Thanks!
Attachment #739099 - Flags: review?(jwalker)
Comment on attachment 739099 [details] [diff] [review]
proposed fix

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

I think we should be more careful about adding executeSoon, it's a trap waiting to happen. Do you know what we're waiting for for each of the added executeSoons or is it just a 'maybe this will help'?
Attachment #739099 - Flags: review?(jwalker)
Attached patch updated patchSplinter Review
(In reply to Joe Walker [:jwalker] from comment #47)
> Comment on attachment 739099 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 739099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should be more careful about adding executeSoon, it's a trap
> waiting to happen.

Agreed. The attempt here was not to use executeSoon() as a way to delay things for an undefined amount of time. It looked to me that layout wasn't flushed, or that responsive mode still had other things to do after emitting "on" and "off" events.

> Do you know what we're waiting for for each of the added
> executeSoons or is it just a 'maybe this will help'?

I updated the patch and picked more judiciously which executeSoons I add. I add one executeSoon before each responsive mode toggle. Each executeSoon is now commented. Also, do note the test had two executeSoon calls to get out of the current event loop, but it is not consistent about it. It also lacked on/off event listeners in a couple of cases. This patch only makes init/destroy consistent through out the test.

I looked in the responsive mode code and I did not see any other events we can rely on. Given that responsive mode code does DOM changes during init/destroy we could only rely on DOM events (which is flimsy), mozRequestAnimationFrame or executeSoon. Am I missing anything?

Please let me know if you have any suggestions. Thank you!
Attachment #739099 - Attachment is obsolete: true
Attachment #739522 - Flags: review?(jwalker)
Comment on attachment 739522 [details] [diff] [review]
updated patch

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

Thanks!
Attachment #739522 - Flags: review?(jwalker) → review+
Thank you for the review.

Landed in inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/474291134a4a
https://hg.mozilla.org/mozilla-central/rev/474291134a4a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch another patchSplinter Review
This test is still failing. Here's a patch that tries to fix the intermittent failure again.

Approach:

- on my system I can no longer reproduce the intermittent failures.
- to reproduce them I removed all the executeSoon() calls and tried to find a way to fix the failures.
- waitForFocus(), wait for DOM events, mozRequestAnimationFrame() did not work.
- what worked is in this patch: force a document reflow by reading document.height.
- this patch is minimal: I did not remove the executeSoon() calls, I only added the line which forces the reflow - to avoid other surprises.

Not much point in asking for review, it's a one-liner. Going to land this in inbound.
https://hg.mozilla.org/mozilla-central/rev/576ff91b64d7
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.