Bug 194189 – Validate navigation policy decisions to avoid crashes in continueLoadAfterNavigationPolicy

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ryosuke Niwa]

We're hitting crashes in FrameLoader::continueLoadAfterNavigationPolicy
where isBackForwardLoadType would return true yet history().provisionalItem() is null.

We think this is because we're mixing up one policy decision requests / responses for different navigations.

Add identifiers for each policy decision in WebCore as done in WebKit2.

<rdar://problem/22872341>

[Ryosuke Niwa]

Created attachment 360967 [details]
Adds the validation

[Ryosuke Niwa]

Created attachment 360970 [details]
Updated for trunk

[EWS Watchlist]

Attachment 360970 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:763:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:910:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:923:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:96:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/loader/PolicyChecker.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:877:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:909:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:921:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/loader/DocumentLoader.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Seems like a real build failure:

C:\cygwin\home\buildbot\WebKit\Source\WebKitLegacy\win\WebCoreSupport\WebFrameLoaderClient.h(103): error C3668: 'WebFrameLoaderClient::dispatchDecidePolicyForResponse': method with override specifier 'override' did not override any base class methods (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebKitLegacy\win\Plugins\PluginView.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]

[Geoffrey Garen]

Comment on attachment 360970 [details]
Updated for trunk

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

r=me, please fix the Windows build

> Source/WebCore/loader/FrameLoaderTypes.h:72
> +    PolicyCheckIdentifier() = default;

Is it valid to construct a null PolicyCheckIdentifier? Is it necessary? Maybe we should = delete this.

> Source/WebCore/loader/FrameLoaderTypes.h:75
> +    static PolicyCheckIdentifier generate();

I think we usually call this function "create".

> Source/WebCore/loader/FrameLoaderTypes.h:89
> +    uint64_t m_check { 0 };

I would call this m_policyCheck.

> Source/WebCore/loader/PolicyChecker.cpp:84
> +    RELEASE_ASSERT_WITH_MESSAGE(m_check, "Received a non-generated policy check identifier");

Maybe call this a null policy check identifier? Not clear what "non-generated" means, and in theory some non-generated values might be non-zero.

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 360970 [details]
> Updated for trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360970&action=review
> 
> r=me, please fix the Windows build

will do.

> > Source/WebCore/loader/FrameLoaderTypes.h:72
> > +    PolicyCheckIdentifier() = default;
> 
> Is it valid to construct a null PolicyCheckIdentifier? Is it necessary?
> Maybe we should = delete this.

Unfortunately we need this in a few places like argument encoders and WebFrame in WebKit2 where we have it as a member.

> > Source/WebCore/loader/FrameLoaderTypes.h:75
> > +    static PolicyCheckIdentifier generate();
> 
> I think we usually call this function "create".

Sure, will rename.

> > Source/WebCore/loader/FrameLoaderTypes.h:89
> > +    uint64_t m_check { 0 };
> 
> I would call this m_policyCheck.

Will do.

> > Source/WebCore/loader/PolicyChecker.cpp:84
> > +    RELEASE_ASSERT_WITH_MESSAGE(m_check, "Received a non-generated policy check identifier");
> 
> Maybe call this a null policy check identifier? Not clear what
> "non-generated" means, and in theory some non-generated values might be
> non-zero.

Sure, will do.

[Ryosuke Niwa]

Created attachment 360993 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 360993 [details]
Patch for landing

Wait for EWS.

[EWS Watchlist]

Attachment 360993 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:763:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:910:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:923:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:96:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/loader/PolicyChecker.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.cpp:627:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:877:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:909:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:921:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/loader/DocumentLoader.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 360997 [details]
Another windows build fix attempt

[EWS Watchlist]

Attachment 360997 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:763:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:910:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:923:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:96:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/loader/PolicyChecker.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.cpp:627:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:877:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:909:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:921:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/loader/DocumentLoader.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 360998 [details]
Fixed Windows build

[EWS Watchlist]

Attachment 360998 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:763:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:910:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:923:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/EmptyFrameLoaderClient.h:96:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/loader/PolicyChecker.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.cpp:627:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:877:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:909:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:921:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/loader/DocumentLoader.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 360998 [details]
Fixed Windows build

Clearing flags on attachment: 360998

Committed r240909: <https://trac.webkit.org/changeset/240909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Comment on attachment 360998 [details]
Fixed Windows build

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

> Source/WebCore/loader/PolicyChecker.cpp:196
> +        if (!responseIdentifier.isValidFor(requestIdentifier))
> +            return;

Why do we not call function in this case?

> Source/WebCore/loader/PolicyChecker.cpp:237
> +        if (!responseIdentifier.isValidFor(requestIdentifier))
> +            return;

Ditto.  These could cause hangs.

[Alex Christensen]

See https://bugs.webkit.org/show_bug.cgi?id=194823