Bug 224196 – Reduce crash inside getAuditToken

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Reduce crash inside getAuditToken

[Alex Christensen]

Created attachment 425189 [details]
Patch

[Alex Christensen]

Created attachment 425190 [details]
Patch

[Alex Christensen]

rdar://74536285

[Alex Christensen]

(this requires an internal change too)

[Alex Christensen]

Should also fix rdar://76098821

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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).

[Alex Christensen]

(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

[Alex Christensen]

Created attachment 425197 [details]
Patch

[Chris Dumez]

(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.

[Alex Christensen]

Created attachment 425198 [details]
Patch

[Alex Christensen]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.)

[Alex Christensen]

Internal change 2250c632d2c2dc873210dd261eabea361fdf600d makes this not break the build.

[EWS]

Committed r275486: <https://commits.webkit.org/r275486>

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

[Radar WebKit Bug Importer]

<rdar://problem/76250706>

[Alex Christensen]

r275522

[Alex Christensen]

r275531