| Summary: | Reduce crash inside getAuditToken | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, ddkilzer, ews-watchlist, jiewen_tan, katherine_cheney, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alex Christensen
2021-04-05 12:20:00 PDT
Created attachment 425189 [details]
Patch
Created attachment 425190 [details]
Patch
(this requires an internal change too) Should also fix rdar://76098821 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 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). (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 Created attachment 425197 [details]
Patch
(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. Created attachment 425198 [details]
Patch
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. (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. (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. (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.) Internal change 2250c632d2c2dc873210dd261eabea361fdf600d makes this not break the build. Committed r275486: <https://commits.webkit.org/r275486> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425198 [details]. |