Bug 234447 – null ptr deref while trying to access DeferredPromise::promise()

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Gabriel Nava Marino]

This occurs after a given DeferredPromise has had its context destroyed in Document::~Document.

[Gabriel Nava Marino]

Created attachment 447487 [details]
Patch

[Gabriel Nava Marino]

<rdar://problem/83069008>

[Chris Dumez]

Comment on attachment 447487 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47
> +    if (shouldIgnoreRequestToFulfill())

Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach.

[Chris Dumez]

Comment on attachment 447487 [details]
Patch

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

Seems like this needs a test to land?

>> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47
>> +    if (shouldIgnoreRequestToFulfill())
> 
> Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach.

Maybe you can use the isEmpty() check instead?

[Gabriel Nava Marino]

Created attachment 448855 [details]
Patch

[Gabriel Nava Marino]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 447487 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447487&action=review
> 
> Seems like this needs a test to land?
> 
> >> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47
> >> +    if (shouldIgnoreRequestToFulfill())
> > 
> > Per the name shouldIgnoreRequestToFulfill() is about ignoring requests to fulfill (meaning resolve or reject) the promise. Using is as a condition in the promise() getter doesn't seem like the appropriate approach.
> 
> Maybe you can use the isEmpty() check instead?

Thank you for the feedback, I have updated the change to use isEmpty() instead.

I have also attached a test case which catches the issue.

[Gabriel Nava Marino]

Created attachment 448858 [details]
Patch

[Darin Adler]

Comment on attachment 448858 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47
> +    if (isEmpty())

Other functions in this class use shouldIgnoreRequestToFulfill rather than calling isEmpty directly. Did you consider that?

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #8)
> Comment on attachment 448858 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448858&action=review
> 
> > Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:47
> > +    if (isEmpty())
> 
> Other functions in this class use shouldIgnoreRequestToFulfill rather than
> calling isEmpty directly. Did you consider that?

Yes, the original proposal used shouldIgnoreRequestToFulfill(), but I have updated to isEmpty() per https://bugs.webkit.org/show_bug.cgi?id=234447#c3.

[Darin Adler]

I see now: sorry for not reviewing the comments before reviewing.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #10)
> I see now: sorry for not reviewing the comments before reviewing.

No problem. Thank you for the review!

[Gabriel Nava Marino]

Created attachment 448880 [details]
Patch

[Gabriel Nava Marino]

Created attachment 448959 [details]
Patch

[Gabriel Nava Marino]

Update TestExpectations to skip the test on iOS (as well as windows) as pointer-lock, which this test relies on, is not supported there.

[Darin Adler]

Comment on attachment 448959 [details]
Patch

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

> LayoutTests/platform/ios/TestExpectations:181
> +fast/js-promise/js-promise-invalid-context-access.html [ Skip ]

Seems really unfortunate that this test depends on pointer-lock.

[EWS]

Committed r287993 (246020@main): <https://commits.webkit.org/246020@main>

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