Bug 214745 - CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections
Summary: CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::WebFram...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-24 09:34 PDT by Kate Cheney
Modified: 2020-07-28 09:20 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2020-07-24 09:44 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-07-24 09:34:29 PDT
WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections() is crashing.
Comment 1 Kate Cheney 2020-07-24 09:34:45 PDT
<rdar://66018965>
Comment 2 Kate Cheney 2020-07-24 09:44:37 PDT
Created attachment 405160 [details]
Patch
Comment 3 Chris Dumez 2020-07-24 09:54:43 PDT
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?
Comment 4 Kate Cheney 2020-07-24 10:56:48 PDT
(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.
Comment 5 Alexey Proskuryakov 2020-07-24 11:04:50 PDT
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.
Comment 6 Kate Cheney 2020-07-24 13:06:48 PDT
(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.
Comment 7 Alexey Proskuryakov 2020-07-24 14:00:06 PDT
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.
Comment 8 Chris Dumez 2020-07-24 14:13:22 PDT
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.
Comment 9 Kate Cheney 2020-07-24 14:17:16 PDT
(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?
Comment 10 Alexey Proskuryakov 2020-07-24 14:18:21 PDT
> a very recent regression from Kate adding this shouldEnableInAppBrowserPrivacyProtections() check

OK then.
Comment 11 EWS 2020-07-24 14:32:24 PDT
Committed r264859: <https://trac.webkit.org/changeset/264859>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405160 [details].
Comment 12 Alexey Proskuryakov 2020-07-24 18:15:23 PDT
What is the next step for addressing the root cause?
Comment 13 Kate Cheney 2020-07-28 09:20:31 PDT
(In reply to Alexey Proskuryakov from comment #12)
> What is the next step for addressing the root cause?

<rdar://problem/66221089>