Bug 209836

Summary: Requests for messageHandlers() in the DOMWindow should be ignored for non-app-bound navigations
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, cdumez, darin, ews-watchlist, japhet, thorton, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kate Cheney 2020-03-31 15:05:48 PDT 
The call to messageHandlers() in the DOMWindow should have protections for non-app-bound navigations.
Comment 1 Kate Cheney 2020-03-31 15:06:48 PDT 
<rdar://problem/61071607>
Comment 2 Kate Cheney 2020-03-31 15:20:59 PDT 
Created attachment 395097 [details]
Patch
Comment 3 Darin Adler 2020-03-31 15:33:35 PDT 
Comment on attachment 395097 [details]
Patch

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

> Source/WebCore/page/WebKitNamespace.cpp:53
> +    if (frame()->loader().client().hasNavigatedAwayFromAppBoundDomain()) {

What guarantees frame() is non-null?
Comment 4 Kate Cheney 2020-03-31 15:39:34 PDT 
(In reply to Darin Adler from comment #3)
> Comment on attachment 395097 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395097&action=review
> 
> > Source/WebCore/page/WebKitNamespace.cpp:53
> > +    if (frame()->loader().client().hasNavigatedAwayFromAppBoundDomain()) {
> 
> What guarantees frame() is non-null?

Nothing, I will add a check here.
Comment 5 Kate Cheney 2020-03-31 15:43:40 PDT 
Created attachment 395099 [details]
Patch
Comment 6 Chris Dumez 2020-03-31 17:19:16 PDT 
Comment on attachment 395099 [details]
Patch

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

> Source/WebCore/page/WebKitNamespace.cpp:33
> +#define RELEASE_LOG_ERROR_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_ERROR_IF(frame()->isAlwaysOnLoggingAllowed(), channel, "%p - WebKitNamespace::" fmt, this, ##__VA_ARGS__)

The frame() deference in here without null check is a foot fun and is bound to introduce crashes in the future. Please add a null check for frame() inside the macro.
Comment 7 Kate Cheney 2020-03-31 17:26:08 PDT 
Created attachment 395111 [details]
Patch
Comment 8 Kate Cheney 2020-03-31 17:27:01 PDT 
(In reply to Chris Dumez from comment #6)
> Comment on attachment 395099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395099&action=review
> 
> > Source/WebCore/page/WebKitNamespace.cpp:33
> > +#define RELEASE_LOG_ERROR_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_ERROR_IF(frame()->isAlwaysOnLoggingAllowed(), channel, "%p - WebKitNamespace::" fmt, this, ##__VA_ARGS__)
> 
> The frame() deference in here without null check is a foot fun and is bound
> to introduce crashes in the future. Please add a null check for frame()
> inside the macro.

Oops missed this one. Thanks for pointing it out!
Comment 9 Brent Fulgham 2020-03-31 17:32:28 PDT 
Comment on attachment 395111 [details]
Patch

I love that 90% of this patch is test case. :-)
Comment 10 Brent Fulgham 2020-03-31 17:33:18 PDT 
Comment on attachment 395111 [details]
Patch

r=me
Comment 11 Kate Cheney 2020-03-31 18:37:14 PDT 
(In reply to Brent Fulgham from comment #9)
> Comment on attachment 395111 [details]
> Patch
> 
> I love that 90% of this patch is test case. :-)

I know, such a tiny code change!

Thanks for the review!
Comment 12 EWS 2020-03-31 18:42:58 PDT 
Committed r259331: <https://trac.webkit.org/changeset/259331>

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