Closed Bug 1362903 Opened 3 years ago Closed 2 years ago

docshell/base/crashtests/1341657.html does not finish properly

Categories

(Core :: Document Navigation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: freesamael)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

From a log:
https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=96824405&lineNumber=1913

00:22:12     INFO - REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/docshell/base/crashtests/1341657.html
00:22:12     INFO - REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/docshell/base/crashtests/1341657.html | 20 / 3195 (0%)
00:22:12     INFO - REFTEST None
00:22:12     INFO - --DOCSHELL 0x127d8a800 == 14 [pid = 1705] [id = {aa70f139-8180-5245-901c-2e7b52862eed}]
00:22:12     INFO - REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/dom/animation/test/crashtests/1239889-1.html
00:22:12     INFO - ++DOMWINDOW == 86 (0x127d97000) [pid = 1705] [serial = 86] [outer = 0x123727800]
00:22:12     INFO - REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/dom/animation/test/crashtests/1239889-1.html | 21 / 3195 (0%)
00:22:12    ERROR - REFTEST ERROR | file:///builds/slave/test/build/tests/reftest/tests/dom/animation/test/crashtests/1239889-1.html | program error managing timeouts
00:22:12    ERROR - 

We processed a subsequent reftest (1239889-1.html) before 1341657.html finished.
Maybe Ehsan can take a look since it seems like he wrote this test.
Flags: needinfo?(ehsan)
No I didn't, Hiro did.  :-)
Flags: needinfo?(ehsan)
:hiro, looks this is something we'd like to get it done soon-ish to unblock your work on bug 1343884. Are you planning to work on this? Feel free to comment if you need assistance. :)
Flags: needinfo?(hikezoe)
Priority: -- → P2
OK, I can take this.  But who is the right person for review?  I understand Ehsan has been very busy for Quantum Flow and I don't want to bother him. Hsin-Yi, suggestions?

Anyway, this is a try with attachment 8866616 [details]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6427bb47d8810b6ebaf724ab12871b681edf04ea

It seems to work.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe) → needinfo?(htsai)
Hi Olli, are you able to help review Hiro's patch?
Flags: needinfo?(htsai) → needinfo?(bugs)
Why does the paint help? Layout flush doesn't mean paint.
Don't you want async finish + requestAnimationFrame or something?
Flags: needinfo?(bugs)
Maybe I don't understand what the issue with the test is.
(In reply to Olli Pettay [:smaug] from comment #8)
> Why does the paint help? Layout flush doesn't mean paint.

Honestly I don't exactly know the reason why a subsequent test is proceeded even if this test is finished.
What I know, what I assume is that there is no trigger to finish this test.

> Don't you want async finish + requestAnimationFrame or something?

Yes, actually I did a try with reftest-wait, and it worked as well. But after the try I found below comment in README.txt [1].

 Note that in layout tests it is often enough to trigger layout using 
    document.body.offsetWidth  // HTML example

 When possible, you should use this technique instead of making your test async.

[1] https://hg.mozilla.org/mozilla-central/file/3b96f2773257/layout/tools/reftest/README.txt#l428
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > Why does the paint help? Layout flush doesn't mean paint.
> 
> Honestly I don't exactly know the reason why a subsequent test is proceeded
> even if this test is finished.

is *not* finished.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > Why does the paint help? Layout flush doesn't mean paint.
> 
> Honestly I don't exactly know the reason why a subsequent test is proceeded
> even if this test is finished.
> What I know, what I assume is that there is no trigger to finish this test.
> 
> > Don't you want async finish + requestAnimationFrame or something?
> 
> Yes, actually I did a try with reftest-wait, and it worked as well. But
> after the try I found below comment in README.txt [1].
> 
>  Note that in layout tests it is often enough to trigger layout using 
>     document.body.offsetWidth  // HTML example
> 
>  When possible, you should use this technique instead of making your test
> async.

After some more thought, I started to think that we should use reftest-wait to avoid not running the script unexpectedly but pass the crash test.

A try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1bab80408e1ef6ba91191199fbb9fc84f0a99d6
Comment on attachment 8866616 [details]
Bug 1362903 - Use reftest-wait to ensure this test finishes.

https://reviewboard.mozilla.org/r/138224/#review155032

rs+
Attachment #8866616 - Flags: review?(bugs) → review+
Thank you for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53d27064c64e
Use reftest-wait to ensure this test finishes. r=smaug
https://hg.mozilla.org/mozilla-central/rev/53d27064c64e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I finally got a rr recording of this failure and hopefully to get more clues from it.
Assignee: hikezoe → sawang
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8886465 [details] [diff] [review]
clear gCurrentURL on reftest finishes

Hi Joel,

I found gCurrentURL didn't get cleared on test finishes. In 1331295.html there's a window.location.reload, so it can occasionally pass twice if gCurrentURL hasn't been updated yet, and the 2nd "reftest:TestDone" causes reftest.jsm think 1341657.html has finished, and eventually cause bug 1343884.

I tried 100 runs on linux64/linux64-qr and the only 3 fails seems to be caused by bug 1204281.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9783c406f1f8784aa48950b60354dd442a7a47db

I'd like to see if this simple change fixes the issue.
Attachment #8886465 - Flags: review?(jmaher)
Comment on attachment 8886465 [details] [diff] [review]
clear gCurrentURL on reftest finishes

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

this looks pretty safe.
Attachment #8886465 - Flags: review?(jmaher) → review+
Attachment #8866616 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c038d1ebf74f
Clear gCurrentURL on reftest finishes. r=jmaher
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c038d1ebf74f
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Depends on: 1381839
Depends on: 1381283
You need to log in before you can comment on or make changes to this bug.