Requests for messageHandlers() in the DOMWindow should be ignored for non-app-bound navigations

[Kate Cheney]

The call to messageHandlers() in the DOMWindow should have protections for non-app-bound navigations.

[Kate Cheney]

<rdar://problem/61071607>

[Kate Cheney]

Created attachment 395097 [details]
Patch

[Darin Adler]

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]

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

Created attachment 395099 [details]
Patch

[Chris Dumez]

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]

Created attachment 395111 [details]
Patch

[Kate Cheney]

(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 on attachment 395111 [details]
Patch

I love that 90% of this patch is test case. :-)

[Brent Fulgham]

Comment on attachment 395111 [details]
Patch

r=me

[Kate Cheney]

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

Committed r259331: <https://trac.webkit.org/changeset/259331>

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