Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear
rdar://problem/64494037
Created attachment 402625 [details] Patch
Comment on attachment 402625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402625&action=review Null check looks fine. > LayoutTests/fast/rendering/iframe-window-animation-modifies-iframe-srcdoc-crash.html:22 > +<video onloadstart="runTest()"> This is running after DumpRenderTree has already finished the test. If you move the testRunner.dumpAsText call to outside the function it should make the results the same for DumpRenderTree and WebKitTestRunner.
Created attachment 402666 [details] Patch
Comment on attachment 402666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402666&action=review > LayoutTests/fast/rendering/iframe-window-animation-modifies-iframe-srcdoc-crash.html:21 > +<video onloadstart="runTest()"> Alex's suggestion helped, but I think we can do even better here. From the test harness's perspective by default, the test is over when you hit the closing </html> tag. So, the video loadstart and requestAnimation frame callbacks, which happen asynchronously on a delay, happen after the test harness thinks the test is over. That seems to work, but it's a bit fragile, since the test harness would be well within its rights to just terminate the test before any of that code ran. testRunner.waitUntilDone() is how we tell the test harness that we want it to wait until some point after the closing </html> tag. And testRunner.notifyDone() is how we tell the test harness we have reached that point. So, I think you should call testRunner.waitUntilDone() at the top, right after testRunner.dumpAsText(), and then call testRunner.notifyDone() as the last line in srcDocModify().
Comment on attachment 402666 [details] Patch r=me since the test does work for now, and we're having a heck of a time getting waitUntilDone() to work
Committed r263473: <https://trac.webkit.org/changeset/263473> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402666 [details].
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 402666 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402666&action=review > > > LayoutTests/fast/rendering/iframe-window-animation-modifies-iframe-srcdoc-crash.html:21 > > +<video onloadstart="runTest()"> > > Alex's suggestion helped, but I think we can do even better here. > > From the test harness's perspective by default, the test is over when you > hit the closing </html> tag. So, the video loadstart and requestAnimation > frame callbacks, which happen asynchronously on a delay, happen after the > test harness thinks the test is over. That seems to work, but it's a bit > fragile, since the test harness would be well within its rights to just > terminate the test before any of that code ran. > > testRunner.waitUntilDone() is how we tell the test harness that we want it > to wait until some point after the closing </html> tag. And > testRunner.notifyDone() is how we tell the test harness we have reached that > point. > > So, I think you should call testRunner.waitUntilDone() at the top, right > after testRunner.dumpAsText(), and then call testRunner.notifyDone() as the > last line in srcDocModify(). Making a note for future reference - waitUntilDone() and notifyDone() was not working for this test. notifyDone was never getting called.
Comment on attachment 402666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402666&action=review >>> LayoutTests/fast/rendering/iframe-window-animation-modifies-iframe-srcdoc-crash.html:21 >>> +<video onloadstart="runTest()"> >> >> Alex's suggestion helped, but I think we can do even better here. >> >> From the test harness's perspective by default, the test is over when you hit the closing </html> tag. So, the video loadstart and requestAnimation frame callbacks, which happen asynchronously on a delay, happen after the test harness thinks the test is over. That seems to work, but it's a bit fragile, since the test harness would be well within its rights to just terminate the test before any of that code ran. >> >> testRunner.waitUntilDone() is how we tell the test harness that we want it to wait until some point after the closing </html> tag. And testRunner.notifyDone() is how we tell the test harness we have reached that point. >> >> So, I think you should call testRunner.waitUntilDone() at the top, right after testRunner.dumpAsText(), and then call testRunner.notifyDone() as the last line in srcDocModify(). > > Making a note for future reference - waitUntilDone() and notifyDone() was not working for this test. notifyDone was never getting called. I do not think this is correct. Just follow what Geoffrey said in his comment and the test will not timeout. For quicker verification, you can just add alert messages in runTest() and srcDocModify() and open the test in mini browser and you will see the alert messages pop up as expected.
We should not leave this broken test in the tree.
(In reply to Simon Fraser (smfr) from comment #10) > We should not leave this broken test in the tree. Hi Simon, Said, Tried making modification to the test as suggested by Geoff and for some reason the test times out for notifyDone with WebKitTestRunner. Even Geoff tried the same and both of us were facing the same issue. I am trying to check on the same. Thanks, Pinki
Reopening to attach new patch.
Created attachment 402800 [details] Patch
Committed r263533: <https://trac.webkit.org/changeset/263533> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402800 [details].