Closed Bug 1111628 Opened 6 years ago Closed 6 years ago

Intermittent test_fullscreen-api.html | Test attempted to use a flaky timeout value 100

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: ehsan)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

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
2.36 KB, patch
cpearce
: review-
Details | Diff | Splinter Review
White-list the non-zero timeout usage in file_fullscreen-utils.js
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...
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.
:ehsan, do you have any ideas for how to deal with this (comment 13)?
Flags: needinfo?(ehsan.akhgari)
(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)
(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)
(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 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-
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.
Attachment #8539562 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/37b71b9bdb3e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.