Bug 224196 - Reduce crash inside getAuditToken
Summary: Reduce crash inside getAuditToken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-05 12:20 PDT by Alex Christensen
Modified: 2021-04-06 10:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.87 KB, patch)
2021-04-05 12:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2021-04-05 12:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2021-04-05 13:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.51 KB, patch)
2021-04-05 13:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-04-05 12:20:00 PDT
Reduce crash inside getAuditToken
Comment 1 Alex Christensen 2021-04-05 12:50:53 PDT
Created attachment 425189 [details]
Patch
Comment 2 Alex Christensen 2021-04-05 12:51:35 PDT
Created attachment 425190 [details]
Patch
Comment 3 Alex Christensen 2021-04-05 12:53:42 PDT
rdar://74536285
Comment 4 Alex Christensen 2021-04-05 12:53:53 PDT
(this requires an internal change too)
Comment 5 Alex Christensen 2021-04-05 13:02:27 PDT
Should also fix rdar://76098821
Comment 6 David Kilzer (:ddkilzer) 2021-04-05 13:08:08 PDT
Comment on attachment 425190 [details]
Patch

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

r=me with additional logging added as requested.  Other changes are optional (but recommended).

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:175
> +        auto auditToken = auxiliaryProcess.parentProcessConnection()->getAuditToken();
> +        if (!auditToken) {
> +            ASSERT_NOT_REACHED();

We should log here if this happens, as it's never supposed to happen according to deleted code above:

    RELEASE_ASSERT(parentAuditToken); // This should be impossible.

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:235
> +        RefPtr<IPC::Connection> connection = auxiliaryProcess.parentProcessConnection();
> +        if (!connection) {
> +            ASSERT_NOT_REACHED();

We should log here if this happens.

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:241
> +        auto auditToken = connection->getAuditToken();
> +        if (!auditToken) {
> +            ASSERT_NOT_REACHED();

We should log here if this happens, as it's never supposed to happen according to deleted code above:

    RELEASE_ASSERT(parentAuditToken); // This should be impossible.

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:56
>  static bool isWebBrowser()
>  {
> -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> -    return false;
> +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
>  }

We should inline this within the source file.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3832
>  bool WebPage::isParentProcessAWebBrowser() const
>  {
>  #if HAVE(AUDIT_TOKEN)
> -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
>  #endif
>      return false;
>  }

We should move this implementation to the header.
Comment 7 David Kilzer (:ddkilzer) 2021-04-05 13:15:28 PDT
Comment on attachment 425190 [details]
Patch

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

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:56
>>  }
> 
> We should inline this within the source file.

Or just change it to an inline local function (non-static).
Comment 8 Alex Christensen 2021-04-05 13:19:18 PDT
(In reply to David Kilzer (:ddkilzer) from comment #6)
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3832
> >  bool WebPage::isParentProcessAWebBrowser() const
> >  {
> >  #if HAVE(AUDIT_TOKEN)
> > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> >  #endif
> >      return false;
> >  }
> 
> We should move this implementation to the header.

I disagree because then we would need to include the definitions of isParentProcessAFullWebBrowser and WebProcess::singleton everywhere we include WebPage.h
Comment 9 Alex Christensen 2021-04-05 13:23:22 PDT
Created attachment 425197 [details]
Patch
Comment 10 Chris Dumez 2021-04-05 13:25:12 PDT
(In reply to David Kilzer (:ddkilzer) from comment #6)
> Comment on attachment 425190 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425190&action=review
> 
> r=me with additional logging added as requested.  Other changes are optional
> (but recommended).
> 
> > Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:175
> > +        auto auditToken = auxiliaryProcess.parentProcessConnection()->getAuditToken();
> > +        if (!auditToken) {
> > +            ASSERT_NOT_REACHED();
> 
> We should log here if this happens, as it's never supposed to happen
> according to deleted code above:
> 
>     RELEASE_ASSERT(parentAuditToken); // This should be impossible.
> 
> > Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:235
> > +        RefPtr<IPC::Connection> connection = auxiliaryProcess.parentProcessConnection();
> > +        if (!connection) {
> > +            ASSERT_NOT_REACHED();
> 
> We should log here if this happens.
> 
> > Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm:241
> > +        auto auditToken = connection->getAuditToken();
> > +        if (!auditToken) {
> > +            ASSERT_NOT_REACHED();
> 
> We should log here if this happens, as it's never supposed to happen
> according to deleted code above:
> 
>     RELEASE_ASSERT(parentAuditToken); // This should be impossible.
> 
> > Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:56
> >  static bool isWebBrowser()
> >  {
> > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > -    return false;
> > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> >  }
> 
> We should inline this within the source file.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3832
> >  bool WebPage::isParentProcessAWebBrowser() const
> >  {
> >  #if HAVE(AUDIT_TOKEN)
> > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> >  #endif
> >      return false;
> >  }
> 
> We should move this implementation to the header.

I don't really understand all the comments about inlining here. This does not obviously look like perf-sensitive code to me.
Comment 11 Alex Christensen 2021-04-05 13:31:38 PDT
Created attachment 425198 [details]
Patch
Comment 12 Alex Christensen 2021-04-05 13:32:09 PDT
Comment on attachment 425198 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.h:49
> +#define WEBKIT_PARENT_PROCESS_FULL_WEB_BROWSER_PARAMETER_AUXILIARY_PROCESS 1

I added this temporarily so I don't have to break the internal build when landing this.
Comment 13 David Kilzer (:ddkilzer) 2021-04-05 13:35:00 PDT
(In reply to Alex Christensen from comment #8)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3832
> > >  bool WebPage::isParentProcessAWebBrowser() const
> > >  {
> > >  #if HAVE(AUDIT_TOKEN)
> > > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> > >  #endif
> > >      return false;
> > >  }
> > 
> > We should move this implementation to the header.
> 
> I disagree because then we would need to include the definitions of
> isParentProcessAFullWebBrowser and WebProcess::singleton everywhere we
> include WebPage.h

Okay.  As noted earlier, it was optional.
Comment 14 David Kilzer (:ddkilzer) 2021-04-05 13:49:17 PDT
(In reply to Chris Dumez from comment #10)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > Comment on attachment 425190 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=425190&action=review
> > 
> > r=me with additional logging added as requested.  Other changes are optional
> > (but recommended).
> > 
> > > Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:56
> > >  static bool isWebBrowser()
> > >  {
> > > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > > -    return false;
> > > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> > >  }
> > 
> I don't really understand all the comments about inlining here. This does
> not obviously look like perf-sensitive code to me.

Alex had a good point in Comment #8 (which I had not considered) about not inlining WebPage::isParentProcessAWebBrowser() because it would require including more headers.

Inlining isWebBrowser() was not motivated by performance.

Unless it impacts readability (or there are a LOT of calls to this function), it seems kind of silly to have a one-line static function with a name that is arguably restating what the full method describes.  I think it made more sense to have a separate static function when it contained more logic.

Let me ask a different way--if there was a WebKit patch that extracted a one-line static function like this and it didn't obviously impact performance and readability improvement was a wash, would you r+ it?

+static bool isWebBrowser()
+{
+    return isParentProcessAFullWebBrowser(WebProcess::singleton());
+}


BTW, all three uses of isWebBrowser() are for early returns like this:

    if (!isWebBrowser())
        return;

I guess that's slightly more readable than this?

    if (! isParentProcessAFullWebBrowser(WebProcess::singleton()))
        return;

But again, I don't think a patch to extract all calls to isParentProcessAFullWebBrowser(WebProcess::singleton()) would even be necessary if isWebBrowser() didn't already exist.
Comment 15 David Kilzer (:ddkilzer) 2021-04-05 13:55:47 PDT
(In reply to David Kilzer (:ddkilzer) from comment #14)
> (In reply to Chris Dumez from comment #10)
> > (In reply to David Kilzer (:ddkilzer) from comment #6)
> > > Comment on attachment 425190 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=425190&action=review
> > > 
> > > r=me with additional logging added as requested.  Other changes are optional
> > > (but recommended).
> > > 
> > > > Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:56
> > > >  static bool isWebBrowser()
> > > >  {
> > > > -    if (auto* connection = WebProcess::singleton().parentProcessConnection())
> > > > -        return isParentProcessAFullWebBrowser(connection->getAuditToken());
> > > > -    return false;
> > > > +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> > > >  }
> > > 
> > I don't really understand all the comments about inlining here. This does
> > not obviously look like perf-sensitive code to me.
> 
> Alex had a good point in Comment #8 (which I had not considered) about not
> inlining WebPage::isParentProcessAWebBrowser() because it would require
> including more headers.
> 
> Inlining isWebBrowser() was not motivated by performance.
> 
> Unless it impacts readability (or there are a LOT of calls to this
> function), it seems kind of silly to have a one-line static function with a
> name that is arguably restating what the full method describes.  I think it
> made more sense to have a separate static function when it contained more
> logic.
> 
> Let me ask a different way--if there was a WebKit patch that extracted a
> one-line static function like this and it didn't obviously impact
> performance and readability improvement was a wash, would you r+ it?
> 
> +static bool isWebBrowser()
> +{
> +    return isParentProcessAFullWebBrowser(WebProcess::singleton());
> +}
> 
> 
> BTW, all three uses of isWebBrowser() are for early returns like this:
> 
>     if (!isWebBrowser())
>         return;
> 
> I guess that's slightly more readable than this?
> 
>     if (! isParentProcessAFullWebBrowser(WebProcess::singleton()))
>         return;

Ugh, paste added a space.

> But again, I don't think a patch to extract all calls to
> isParentProcessAFullWebBrowser(WebProcess::singleton()) would even be
> necessary if isWebBrowser() didn't already exist.

I suppose there is a small amount of space savings by doing this, though, which I hadn't considered.

Should we go through WebKit and find source files with N >= 3? (5?) calls to the same method, and move them into static functions?  I'm sure we could start saving a few bytes by doing this.  Or would it be better to look at other strategies to save space?  (Serious question as I hadn't considered this, but maybe others have.)
Comment 16 Alex Christensen 2021-04-05 20:29:09 PDT
Internal change 2250c632d2c2dc873210dd261eabea361fdf600d makes this not break the build.
Comment 17 EWS 2021-04-05 21:02:45 PDT
Committed r275486: <https://commits.webkit.org/r275486>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425198 [details].
Comment 18 Radar WebKit Bug Importer 2021-04-05 21:04:45 PDT
<rdar://problem/76250706>
Comment 19 Alex Christensen 2021-04-06 09:07:02 PDT
r275522
Comment 20 Alex Christensen 2021-04-06 10:28:24 PDT
r275531