Bug 221304 – Null Ptr Deref @ libWebCoreTestSupport.dylib: WebCoreTestSupport::resetInternalsObject

• Status: NEW
• Component: Tools / Tests

[Jonathan Bedard]

A buzzer caught this when using our test harness a few months back, not an urgent change, but the fix is trivial.

[Jonathan Bedard]

<rdar://problem/6301944>

[Jonathan Bedard]

Created attachment 419076 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #0)
> A buzzer caught this when using our test harness a few months back, not an
> urgent change, but the fix is trivial.

*fuzzer, not buzzer

[Alexey Proskuryakov]

Comment on attachment 419076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419076&action=review

> Source/WebCore/testing/js/WebCoreTestSupport.cpp:84
> +    if (!scriptContext)
> +        return;

When can this happen? This function should always run, and if we return early, the next test will misbehave because we didn't reset the settings. I think that adding an early return is not OK, and we need to actually make sure that we cannot get here.

[Jonathan Bedard]

Comment on attachment 419076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419076&action=review

>> Source/WebCore/testing/js/WebCoreTestSupport.cpp:84
>> +        return;
> 
> When can this happen? This function should always run, and if we return early, the next test will misbehave because we didn't reset the settings. I think that adding an early return is not OK, and we need to actually make sure that we cannot get here.

This was reported when using one of our test runners to fuzz WebKit.

It doesn't seem like the scriptContext or the page should ever be null, and if this were production code, I would agree that these should just be assertions. But it does seem more helpful for fuzzers if the crashes from unexpected behavior occur in production code rather than testing code, even at the cost of putting the test harness in a bad state.

[Alexey Proskuryakov]

Seeing the original bug, I do not believe that it's more important than messing up regression tests.

[Jonathan Bedard]

Created attachment 419168 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 419076 [details]
Patch

I'll say r- based on the previous conversation. I think that we need to get to the bottom of this, as silencing the crash with a null check will be more costly in terms of manual work analyzing failures than hitting crashes is.