Closed
Bug 1111628
Opened 11 years ago
Closed 11 years ago
Intermittent test_fullscreen-api.html | Test attempted to use a flaky timeout value 100
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
Tracking | Status | |
---|---|---|
firefox35 | --- | unaffected |
firefox36 | --- | unaffected |
firefox37 | --- | fixed |
firefox-esr31 | --- | unaffected |
People
(Reporter: RyanVM, Assigned: ehsan.akhgari)
Details
(Keywords: intermittent-failure)
Attachments
(2 files)
2.36 KB,
patch
|
cpearce
:
review-
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
Just what I needed, more motivation to disable test_fullscreen-api.html.
19:53:31 INFO - 1130 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Body should be FSE
19:53:31 INFO - 1131 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] FSE should not change
19:53:31 INFO - 1132 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Subdoc container should be FSE.
19:53:31 INFO - 1133 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Subdoc body should be FSE in subdoc
19:53:31 INFO - 1134 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Should only exit fullscreen on a fullscreen doc
19:53:31 INFO - 1135 INFO [rollback] Exited fullscreen
19:53:31 INFO - 1136 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] FSE should rollback to FSE.
19:53:31 INFO - 1137 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Should only exit fullscreen on a fullscreen doc
19:53:31 INFO - 1138 INFO TEST-FAIL | /tests/dom/html/test/test_fullscreen-api.html | fullscreenchange before exiting fullscreen complete! window.fullScreen=false normal=(500,529) outerWidth=1600 width=1600 outerHeight=1200 height=1200
19:53:31 INFO - 1139 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_fullscreen-api.html | Test attempted to use a flaky timeout value 100 - expected PASS
19:53:31 INFO - 1140 INFO [rollback] Exited fullscreen
19:53:31 INFO - 1141 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] Should have left full-screen entirely
19:53:31 INFO - 1142 INFO TEST-FAIL | /tests/dom/html/test/test_fullscreen-api.html | fullscreenchange before entering fullscreen complete! window.fullScreen=true normal=(500,529) outerWidth=500 width=1600 outerHeight=529 height=1200
19:53:31 INFO - 1143 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_fullscreen-api.html | Test attempted to use a flaky timeout value 100 - expected PASS
19:53:31 INFO - [Parent 1864] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file /builds/slave/m-cen-lx-d-0000000000000000000/build/src/docshell/shistory/src/nsSHistory.cpp, line 1339
19:53:31 INFO - 1144 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] FSE should be e('fse')
19:53:31 INFO - 1145 INFO TEST-PASS | /tests/dom/html/test/test_fullscreen-api.html | [rollback] FSE should be e('fse-inner')
19:53:31 INFO - 1146 INFO TEST-FAIL | /tests/dom/html/test/test_fullscreen-api.html | fullscreenchange before exiting fullscreen complete! window.fullScreen=false normal=(500,529) outerWidth=1600 width=1600 outerHeight=1200 height=1200
19:53:31 INFO - 1147 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_fullscreen-api.html | Test attempted to use a flaky timeout value 100 - expected PASS
and more...
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 13•11 years ago
|
||
This test is failing all of the time -- it looks like the white-list mechanism is not effective for this test because the setTimeout happens in another window. I suspect for a similar reason this failure isn't propagated to the summary counts, so these jobs are usually green, and those starred here are detected in conjunction with another failure.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 16•11 years ago
|
||
:ehsan, do you have any ideas for how to deal with this (comment 13)?
Flags: needinfo?(ehsan.akhgari)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #13)
> This test is failing all of the time -- it looks like the white-list
> mechanism is not effective for this test because the setTimeout happens in
> another window. I suspect for a similar reason this failure isn't propagated
> to the summary counts, so these jobs are usually green, and those starred
> here are detected in conjunction with another failure.
The test is not failing all the time. If you look at file_fullscreen-utils.js around workAroundFullscreenTransition, we're just waiting in order for the element to actually become fullscreen. However, waiting 100ms doesn't make any more sense than waiting 0ms would, so I'll just adjust things accordingly.
Assignee: nobody → ehsan.akhgari
Flags: needinfo?(ehsan.akhgari)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8539302 -
Flags: review?(cpearce)
Comment 22•11 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> (In reply to Chris Manchester [:chmanchester] from comment #13)
> > This test is failing all of the time -- it looks like the white-list
> > mechanism is not effective for this test because the setTimeout happens in
> > another window. I suspect for a similar reason this failure isn't propagated
> > to the summary counts, so these jobs are usually green, and those starred
> > here are detected in conjunction with another failure.
>
> The test is not failing all the time. If you look at
> file_fullscreen-utils.js around workAroundFullscreenTransition, we're just
> waiting in order for the element to actually become fullscreen. However,
> waiting 100ms doesn't make any more sense than waiting 0ms would, so I'll
> just adjust things accordingly.
Right, I should have said that SimpleTest attempts to log this as a failure due to a flaky timeout all of the time, but doesn't get as far as turning the job orange -- if you click on any green M-2 job in Treeherder this failure output is visible in the failure summary. My question is whether the detection of flaky timeouts needs to be changed to handle this case better.
Flags: needinfo?(ehsan.akhgari)
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #22)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #20)
> > (In reply to Chris Manchester [:chmanchester] from comment #13)
> > > This test is failing all of the time -- it looks like the white-list
> > > mechanism is not effective for this test because the setTimeout happens in
> > > another window. I suspect for a similar reason this failure isn't propagated
> > > to the summary counts, so these jobs are usually green, and those starred
> > > here are detected in conjunction with another failure.
> >
> > The test is not failing all the time. If you look at
> > file_fullscreen-utils.js around workAroundFullscreenTransition, we're just
> > waiting in order for the element to actually become fullscreen. However,
> > waiting 100ms doesn't make any more sense than waiting 0ms would, so I'll
> > just adjust things accordingly.
>
> Right, I should have said that SimpleTest attempts to log this as a failure
> due to a flaky timeout all of the time, but doesn't get as far as turning
> the job orange -- if you click on any green M-2 job in Treeherder this
> failure output is visible in the failure summary. My question is whether the
> detection of flaky timeouts needs to be changed to handle this case better.
I think the reason is that this test opens up HTML windows which include SimpleTest.js but they are not in fact a mochitest (which is kind of not what you're supposed to do), so there is no parent runner for us to report the failure to, so we just dump the failure.
Flags: needinfo?(ehsan.akhgari)
Comment 24•11 years ago
|
||
Comment on attachment 8539302 [details] [diff] [review]
Wait 0ms for the element to become fullscreen in case we need to do the Linux specific workaround instead of the made-up 100ms value
Review of attachment 8539302 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/html/test/file_fullscreen-utils.js
@@ +58,5 @@
> " window.fullScreen=" + window.fullScreen +
> " normal=(" + normalSize.w + "," + normalSize.h + ")" +
> " outerWidth=" + window.outerWidth + " width=" + window.screen.width +
> " outerHeight=" + window.outerHeight + " height=" + window.screen.height);
> + setTimeout(function(){listener(event);}, 0);
What this function is actually doing is waiting for the fullscreen transition to complete, and only running the listener once it has completed.
We don't have a way yet to ensure that the "fullscreenchange" event doesn't fire before the widget/window actually finishes resizing. So if we change the timeout from 100 to 0, we'll be busy waiting, which seems bad to me.
We should keep this timeout at 100ms, or at least non-zero.
Attachment #8539302 -
Flags: review?(cpearce) → review-
Assignee | ||
Comment 25•11 years ago
|
||
FWIW I understood what this code is trying to do, I just think that the busy wait in this case should be ok, but fine, I'll white-list.
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8539562 -
Flags: review?(cpearce)
Updated•11 years ago
|
Attachment #8539562 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 27•11 years ago
|
||
Comment 28•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Reporter | ||
Updated•11 years ago
|
status-firefox35:
--- → unaffected
status-firefox36:
--- → unaffected
status-firefox37:
--- → fixed
status-firefox-esr31:
--- → unaffected
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•