Bug 214745 – CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections() is crashing.

[Kate Cheney]

<rdar://66018965>

[Kate Cheney]

Created attachment 405160 [details]
Patch

[Chris Dumez]

Comment on attachment 405160 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:-865
> -        for (WebFrame* frame = this; !frame->isMainFrame(); frame = frame->parentFrame()) {

To maintain previous behavior, we should have used:
frame && !frame->isMainFrame()

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:865
> +        for (WebFrame* frame = this; frame; frame = frame->parentFrame()) {

This is a behavior change because we are now going to run the code in the loop for the main frame. Is that OK?

[Kate Cheney]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 405160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405160&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:-865
> > -        for (WebFrame* frame = this; !frame->isMainFrame(); frame = frame->parentFrame()) {
> 
> To maintain previous behavior, we should have used:
> frame && !frame->isMainFrame()
> 
> > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:865
> > +        for (WebFrame* frame = this; frame; frame = frame->parentFrame()) {
> 
> This is a behavior change because we are now going to run the code in the
> loop for the main frame. Is that OK?

Yes, this is ok. Right now we have a separate check in place in WebPageProxy.cpp where if the main frame is not app-bound, we will enable app-bound domain protections for all further loads, so we don't rely on this loop being run for the main frame. But it doesn't hurt to check it here as well.

[Alexey Proskuryakov]

Comment on attachment 405160 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        We should stop iterating if the frame is null, even if it is not
> +        the main frame, to avoid calling functions on a null frame.

This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash.

[Kate Cheney]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 405160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405160&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        We should stop iterating if the frame is null, even if it is not
> > +        the main frame, to avoid calling functions on a null frame.
> 
> This means that we are running JS in a detached frame, which seems like a
> bigger problem that a mere crash.

Agreed, perhaps this should be a separate investigation though.

[Alexey Proskuryakov]

Comment on attachment 405160 [details]
Patch

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

>>> Source/WebKit/ChangeLog:10
>>> +        the main frame, to avoid calling functions on a null frame.
>> 
>> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash.
> 
> Agreed, perhaps this should be a separate investigation though.

I'm not sure if adding the null check will improve customer experience. Running JS in a detached frame will likely crash anyway, just in a different place, but it wold be just as bad for the user. It can also expose a security vulnerability that used to be prevented by the null pointer crash, but turns into a logic error without the crash.

Sometimes we don't know what's happening and spray null pointer checks without much thinking, but this specific case seems to be more suspicious than most.

[Chris Dumez]

Comment on attachment 405160 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:10
>>>> +        the main frame, to avoid calling functions on a null frame.
>>> 
>>> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash.
>> 
>> Agreed, perhaps this should be a separate investigation though.
> 
> I'm not sure if adding the null check will improve customer experience. Running JS in a detached frame will likely crash anyway, just in a different place, but it wold be just as bad for the user. It can also expose a security vulnerability that used to be prevented by the null pointer crash, but turns into a logic error without the crash.
> 
> Sometimes we don't know what's happening and spray null pointer checks without much thinking, but this specific case seems to be more suspicious than most.

This is a top crasher and a very recent regression from Kate adding this shouldEnableInAppBrowserPrivacyProtections() check. I think a fix in this shouldEnableInAppBrowserPrivacyProtections() is very reasonable for such crash.
Whether or not this function was called on detached frames did NOT change when Kate added the shouldEnableInAppBrowserPrivacyProtections() check.

Also note that if we were crashing before Kate's patch then we would have seen a top crasher in this code path. I am not aware that we have. We don't have evidence that this method crashes or does something bad when called on a detached frame. We do have evidence that Kate's recent addition of the shouldEnableInAppBrowserPrivacyProtections() introduced a crash and she is fixing that crash.

[Kate Cheney]

(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 405160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405160&action=review
> 
> >>> Source/WebKit/ChangeLog:10
> >>> +        the main frame, to avoid calling functions on a null frame.
> >> 
> >> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash.
> > 
> > Agreed, perhaps this should be a separate investigation though.
> 
> I'm not sure if adding the null check will improve customer experience.
> Running JS in a detached frame will likely crash anyway, just in a different
> place, but it wold be just as bad for the user. It can also expose a
> security vulnerability that used to be prevented by the null pointer crash,
> but turns into a logic error without the crash.
> 
> Sometimes we don't know what's happening and spray null pointer checks
> without much thinking, but this specific case seems to be more suspicious
> than most.

My thought is that fixing this will return customer experience to whatever it was prior to this code being added, which is probably like you said another crash or protection put in place to prevent JS execution in a detached frame. If it's another crash, we will see it in CrashTracer. If it's a handled error, we probably don't need to do anything. And if it is not addressed, we can fix it in a separate patch. Do you have another idea for addressing this bug?

[Alexey Proskuryakov]

> a very recent regression from Kate adding this shouldEnableInAppBrowserPrivacyProtections() check

OK then.

[EWS]

Committed r264859: <https://trac.webkit.org/changeset/264859>

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

[Alexey Proskuryakov]

What is the next step for addressing the root cause?

[Kate Cheney]

(In reply to Alexey Proskuryakov from comment #12)
> What is the next step for addressing the root cause?

<rdar://problem/66221089>