Bug 213543 - REGRESSION(r260800): Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear
Summary: REGRESSION(r260800): Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-23 18:51 PDT by Pinki Gyanchandani
Modified: 2020-06-25 15:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2020-06-23 23:43 PDT, Pinki Gyanchandani
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2020-06-24 11:42 PDT, Pinki Gyanchandani
no flags Details | Formatted Diff | Diff
Patch (1.66 KB, patch)
2020-06-25 13:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pinki Gyanchandani 2020-06-23 18:51:51 PDT
Null Ptr Deref READ @ WTF::Optional<WTF::Seconds>::clear
Comment 1 Pinki Gyanchandani 2020-06-23 18:53:05 PDT
rdar://problem/64494037
Comment 2 Pinki Gyanchandani 2020-06-23 23:43:51 PDT
Created attachment 402625 [details]
Patch
Comment 3 Alex Christensen 2020-06-24 11:12:51 PDT
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.
Comment 4 Pinki Gyanchandani 2020-06-24 11:42:12 PDT
Created attachment 402666 [details]
Patch
Comment 5 Geoffrey Garen 2020-06-24 12:26:56 PDT
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 6 Geoffrey Garen 2020-06-24 13:45:07 PDT
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
Comment 7 EWS 2020-06-24 14:07:41 PDT
Committed r263473: <https://trac.webkit.org/changeset/263473>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402666 [details].
Comment 8 Pinki Gyanchandani 2020-06-24 15:41:00 PDT
(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 9 Said Abou-Hallawa 2020-06-24 16:32:24 PDT
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.
Comment 10 Simon Fraser (smfr) 2020-06-24 17:14:01 PDT
We should not leave this broken test in the tree.
Comment 11 Pinki Gyanchandani 2020-06-24 17:54:39 PDT
(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
Comment 12 Said Abou-Hallawa 2020-06-25 13:08:58 PDT
Reopening to attach new patch.
Comment 13 Said Abou-Hallawa 2020-06-25 13:08:59 PDT
Created attachment 402800 [details]
Patch
Comment 14 EWS 2020-06-25 15:12:20 PDT
Committed r263533: <https://trac.webkit.org/changeset/263533>

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