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

Kate Cheney
Reported 2020-03-31 15:05:48 PDT
The call to messageHandlers() in the DOMWindow should have protections for non-app-bound navigations.
Attachments
Patch (21.96 KB, patch)
2020-03-31 15:20 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (21.97 KB, patch)
2020-03-31 15:43 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (21.98 KB, patch)
2020-03-31 17:26 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kate Cheney
Comment 1 2020-03-31 15:06:48 PDT
<rdar://problem/61071607>
Kate Cheney
Comment 2 2020-03-31 15:20:59 PDT
Created attachment 395097 [details] Patch
Darin Adler
Comment 3 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?
Kate Cheney
Comment 4 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.
Kate Cheney
Comment 5 2020-03-31 15:43:40 PDT
Created attachment 395099 [details] Patch
Chris Dumez
Comment 6 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.
Kate Cheney
Comment 7 2020-03-31 17:26:08 PDT
Created attachment 395111 [details] Patch
Kate Cheney
Comment 8 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!
Brent Fulgham
Comment 9 2020-03-31 17:32:28 PDT
Comment on attachment 395111 [details] Patch I love that 90% of this patch is test case. :-)
Brent Fulgham
Comment 10 2020-03-31 17:33:18 PDT
Comment on attachment 395111 [details] Patch r=me
Kate Cheney
Comment 11 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!
EWS
Comment 12 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].
Note You need to log in before you can comment on or make changes to this bug.