Closed Bug 594909 Opened 14 years ago Closed 14 years ago

Intermittent failure in browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible

Categories

(Firefox Graveyard :: Panorama, defect, P1)

Product:
Firefox Graveyard
Component:
Panorama
Platform:
x86
Windows 7
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ehsan.akhgari, Assigned: iangilman)

References

Details

(Keywords: intermittent-failure, Whiteboard: [qa-])

Attachments

(1 file, 5 obsolete files)

patch v6
2.20 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284059797.1284061653.5459.gz Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2010/09/09 12:16:37 TEST-START | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js Chrome file doesn't exist: /Users/cltbld/talos-slave/mozilla-central_snowleopard-debug_test-mochitest-other/build/mochitest/browser/browser/base/content/test/tabview/head.js TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View button exists TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Console message: tabview assert: must already be linkedTabItems__update([object XULElement])@chrome://browser/content/tabview.js:4548 TabItems_heartbeat()@chrome://browser/content/tabview.js:4647 TabItems_resumePainting()@chrome://browser/content/tabview.js:4685 ()@chrome://browser/content/tabview.js:4382 (-6)@chrome://browser/content/tabview.js:600 TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359 WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359 WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359 TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test took 1.494s to complete TEST-INFO | checking window state TEST-INFO | before wait for focus -- loaded: complete active window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank desired window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank docshell visible: true TEST-INFO | already focused TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank desired window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank docshell visible: true
(In reply to comment #0) > http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284059797.1284061653.5459.gz > Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on > 2010/09/09 12:16:37 > > TEST-START | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > Chrome file doesn't exist: > /Users/cltbld/talos-slave/mozilla-central_snowleopard-debug_test-mochitest-other/build/mochitest/browser/browser/base/content/test/tabview/head.js Does heads.js generate by the test framework? > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is hidden > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View button exists > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is hidden > WARNING: Overflowed nscoord_MAX in conversion to nscoord: file > ../../dist/include/nsCoord.h, line 359 > TEST-UNEXPECTED-FAIL | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is visible > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is visible > TEST-INFO | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Console message: tabview assert: must already be > linkedTabItems__update([object > XULElement])@chrome://browser/content/tabview.js:4548 > TabItems_heartbeat()@chrome://browser/content/tabview.js:4647 > TabItems_resumePainting()@chrome://browser/content/tabview.js:4685 > ()@chrome://browser/content/tabview.js:4382 > (-6)@chrome://browser/content/tabview.js:600 > > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is hidden > WARNING: Overflowed nscoord_MAX in conversion to nscoord: file > ../../dist/include/nsCoord.h, line 359 > WARNING: Overflowed nscoord_MAX in conversion to nscoord: file > ../../dist/include/nsCoord.h, line 359 > WARNING: Overflowed nscoord_MAX in conversion to nscoord: file > ../../dist/include/nsCoord.h, line 359 > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is visible > TEST-PASS | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Tab View is hidden > TEST-INFO | > chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js > | Test took 1.494s to complete > > TEST-INFO | checking window state > TEST-INFO | before wait for focus -- loaded: complete active window: ([object > ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) > chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper > [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank desired > window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) > chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper > [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank docshell > visible: true > TEST-INFO | already focused > TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active > window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) > chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper > [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank desired > window: ([object ChromeWindow @ 0x10ff01f70 (native @ 0x109ffaa38)]) > chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper > [object Window @ 0x1119e7740 (native @ 0x114ff23c8)]]) about:blank docshell > visible: true
Attached patch v1 (obsolete) — — Splinter Review
Pushed it to try and waiting for the results
Attachment #474392 - Flags: feedback?(ian)
Assignee: nobody → raymond
Passed try
Comment on attachment 474392 [details] [diff] [review] v1 I'm not familiar with .doCommand, but if it does the trick, great! Any idea why .synthesizeMouse wasn't working?
Attachment #474392 - Flags: review?(dietrich)
Attachment #474392 - Flags: feedback?(ian)
Attachment #474392 - Flags: feedback+
Comment on attachment 474392 [details] [diff] [review] v1 If we want to simulate a press on a button, the EventUtils.synthesizeMouse() isn't the best method to use because it causes some issues on some platforms.
Comment on attachment 474392 [details] [diff] [review] v1 synthesized mouse clicks might fail if the app lost focus? regardless, this patch ensures execution, r=me.
Attachment #474392 - Flags: review?(dietrich) → review+
Attachment #474392 - Flags: approval2.0?
Should we be replacing our synthesizeMouse calls in our other tests as well? If so, please make a follow-up.
Comment on attachment 474392 [details] [diff] [review] v1 Test fixes don't need approval. Is this ready to land?
Attachment #474392 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/99c40db2f40e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Looks like this is still happening so reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v2 (obsolete) — — Splinter Review
Attachment #474392 - Attachment is obsolete: true
Attachment #475772 - Flags: feedback?(ian)
Comment on attachment 475772 [details] [diff] [review] v2 Looks like the big change here is the executeSoon? I don't know enough to say whether this is good or bad. Seems worth a try...
Attachment #475772 - Flags: review?(dietrich)
Attachment #475772 - Flags: feedback?(ian)
Attachment #475772 - Flags: feedback+
Priority: -- → P1
Target Milestone: Firefox 4.0b7 → Future
Sorry, didn't mean to assign to future.
Target Milestone: Future → ---
Comment on attachment 475772 [details] [diff] [review] v2 hrm, seems like there should be a deterministic event chain here. i guess r=me to land and see if it actually changes anything. if it does, i think we should still try to track down exactly what is failing and where.
Attachment #475772 - Flags: review?(dietrich) → review+
Attached patch [checked-in] Patch for check-in (obsolete) — — Splinter Review
Attachment #475772 - Attachment is obsolete: true
Comment on attachment 476766 [details] [diff] [review] [checked-in] Patch for check-in http://hg.mozilla.org/mozilla-central/rev/0dcf8c885e07
Attachment #476766 - Attachment description: Patch for check-in → [checked-in] Patch for check-in
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Re-opening as this has just occurred again, and it appears it wasn't certain the previous change would fix it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
After looking into the log, I suspect the hide tabview is called after button.doCommand() but before onTabViewShown() so the TabView.isVisible() returns false. I went through the test (browser_tabview_group.js) which runs before this test, however, couldn't find anything which might cause this. Ian/mitcho/sean: if you have a chance, please have a look to see whether you can find something suspicious. Chrome file doesn't exist: /Users/cltbld/talos-slave/tryserver_snowleopard-debug_test-mochitest-other/build/mochitest/browser/browser/base/content/test/tabview/head.js TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View button exists // NOTE: this shouldn't exist if the test runs correctly. TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible. Count: 0 TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is visible. Count: 1 TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden
OS: Mac OS X → Windows 7
Assignee: raymond → ian
Blocks: 597043
Attached patch patch v1 (obsolete) — — Splinter Review
I have no idea why it would be adding that extra hide in there. At any rate, we can make sure it doesn't do that, by not adding the hide handler until after the initial call, as I've done in this patch. I've also updated some of the test messages to give us a little bit more diagnostic info, and I've removed the executeSoon as it didn't seem to make a difference. I also did a little re-org; I hope you don't mind. Pushed to try: 9b2d7ed348e7
Attachment #476766 - Attachment is obsolete: true
Attachment #481095 - Flags: feedback?(raymond)
Comment on attachment 481095 [details] [diff] [review] patch v1 Looks good.
Attachment #481095 - Flags: review?(dolske)
Attachment #481095 - Flags: feedback?(raymond)
Attachment #481095 - Flags: feedback+
Comment on attachment 481095 [details] [diff] [review] patch v1 Switching the review to Dietrich, as he's already reviewed this bug a few times.
Attachment #481095 - Flags: review?(dolske) → review?(dietrich)
Attachment #481095 - Flags: review?(dietrich) → review+
Try was clean
Attachment #481095 - Flags: approval2.0?
Comment on attachment 481095 [details] [diff] [review] patch v1 Test changes don't need approval. Ship it! :)
Attachment #481095 - Flags: approval2.0?
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/7561c0da2362
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
That try push was on top of http://hg.mozilla.org/mozilla-central/rev/5a41a70eb631 so it should have http://hg.mozilla.org/mozilla-central/rev/7561c0da2362 in it, though I'll wait for somewhere non-try to actually reopen.
Whiteboard: [orange] → [orange][qa-]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v5, a new beginning (obsolete) — — Splinter Review
Seems like the only possible explanation is that the deck doesn't update its selectedIndex until after the doCommand is finished; I've introduced a setTimeout to break the contention.
Attachment #481095 - Attachment is obsolete: true
Attachment #484822 - Flags: feedback?(raymond)
Comment on attachment 484822 [details] [diff] [review] Patch v5, a new beginning Looks like it would fix the issue.
Attachment #484822 - Flags: feedback?(raymond) → feedback+
Comment on attachment 484822 [details] [diff] [review] Patch v5, a new beginning Dietrich, I'm giving this to you for review, as you've reviewed the previous patches for this bug. Let me know if you feel you can't get to it.
Attachment #484822 - Flags: review?(dietrich)
Attached patch patch v6 — — Splinter Review
Made the setTimeout keep trying, per Dietrich via IRC.
Attachment #484822 - Attachment is obsolete: true
Attachment #487446 - Flags: review?(dietrich)
Attachment #484822 - Flags: review?(dietrich)
Comment on attachment 487446 [details] [diff] [review] patch v6 r=me, thanks!
Attachment #487446 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/bcad241e5c22
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
There was a merge problem with the last push. Re pushed: http://hg.mozilla.org/mozilla-central/rev/0951ef6b968f
Comment on attachment 487446 [details] [diff] [review] patch v6 >diff --git a/browser/base/content/test/tabview/browser_tabview_launch.js b/browser/base/content/test/tabview/browser_tabview_launch.js >--- a/browser/base/content/test/tabview/browser_tabview_launch.js >+++ b/browser/base/content/test/tabview/browser_tabview_launch.js >+ setTimeout(waitForSwitch, 10); Ten milliseconds? Was that intended (asking out of curiosity). >+ setTimeout(waitForSwitch, 1); Common convention is to use setTimeout(fun, 0) for a simple (not time-specific) delay.
(In reply to comment #85) > >+ setTimeout(waitForSwitch, 10); > > Ten milliseconds? Was that intended (asking out of curiosity). Yes. I figure if it didn't work right away, give the cpu a few extra cycles rather than checking immediately again. > >+ setTimeout(waitForSwitch, 1); > > Common convention is to use setTimeout(fun, 0) for a simple (not time-specific) > delay. I feel like I've had trouble with that in the past, but I'm not sure when or why.
Tinderbox machines tend to be __very__ slow, so 10 milliseconds is not really the same as on your dev machine. It might just create more overhead. IMHO, it should be a 100-200 value...
(In reply to comment #87) > Tinderbox machines tend to be __very__ slow, so 10 milliseconds is not really > the same as on your dev machine. It might just create more overhead. IMHO, it > should be a 100-200 value... Gotcha. Well, I really am expecting that the first setTimeout will cover it and that that second timeout is just a fallback, so hopefully it's not an issue. I'll definitely keep it in mind for next time. Unless you think we should apply another patch?
(In reply to comment #86) > (In reply to comment #85) > > >+ setTimeout(waitForSwitch, 10); > > > > Ten milliseconds? Was that intended (asking out of curiosity). > > Yes. I figure if it didn't work right away, give the cpu a few extra cycles > rather than checking immediately again. This is wrong. If setTimeout(0) doesn't work, you're doing something wrong. Basically, setTimeout(foo, N) where N>0 is a recipe for intermittent orange. > > >+ setTimeout(waitForSwitch, 1); > > > > Common convention is to use setTimeout(fun, 0) for a simple (not time-specific) > > delay. > > I feel like I've had trouble with that in the past, but I'm not sure when or > why. More specifically, setTimeout(foo, 0) will run foo the next time we hit the event loop, which will be after other events currently in the queue have been processed.
(In reply to comment #89) > > > >+ setTimeout(waitForSwitch, 10); > > > > > > Ten milliseconds? Was that intended (asking out of curiosity). > > > > Yes. I figure if it didn't work right away, give the cpu a few extra cycles > > rather than checking immediately again. > > This is wrong. If setTimeout(0) doesn't work, you're doing something wrong. > > Basically, setTimeout(foo, N) where N>0 is a recipe for intermittent orange. If you look at the code, this is part of a timeout loop where we wait for the status to change. I originally just had the 1 setTimeout (with 1ms), but dietrich suggested that it would be better to keep looping and polling the status. > > > >+ setTimeout(waitForSwitch, 1); > > > > > > Common convention is to use setTimeout(fun, 0) for a simple (not time-specific) > > > delay. > > > > I feel like I've had trouble with that in the past, but I'm not sure when or > > why. > > More specifically, setTimeout(foo, 0) will run foo the next time we hit the > event loop, which will be after other events currently in the queue have been > processed. Yeah, that was my understanding, but I found it didn't work that way somewhere... since I don't remember where, I guess I'll switch back to using 0.
Whiteboard: [orange][qa-] → [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: