Bug 213543 – REGRESSION(r260800): Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Pinki Gyanchandani]

Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear

[Pinki Gyanchandani]

rdar://problem/64494037

[Pinki Gyanchandani]

Created attachment 402625 [details]
Patch

[Alex Christensen]

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.

[Pinki Gyanchandani]

Created attachment 402666 [details]
Patch

[Geoffrey Garen]

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().

[Geoffrey Garen]

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

[EWS]

Committed r263473: <https://trac.webkit.org/changeset/263473>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402666 [details].

[Pinki Gyanchandani]

(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.

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

We should not leave this broken test in the tree.

[Pinki Gyanchandani]

(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

[Said Abou-Hallawa]

Reopening to attach new patch.

[Said Abou-Hallawa]

Created attachment 402800 [details]
Patch

[EWS]

Committed r263533: <https://trac.webkit.org/changeset/263533>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402800 [details].