Bug 231401 - WebKitTestRunner should check mainFrameURL exists in InjectedBundlePage::willSendRequestForFrame
Summary: WebKitTestRunner should check mainFrameURL exists in InjectedBundlePage::will...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-07 16:48 PDT by Gabriel Nava Marino
Modified: 2021-10-12 14:59 PDT (History)
9 users (show)

See Also:

Attachments
Patch (3.83 KB, patch)
2021-10-07 16:57 PDT, Gabriel Nava Marino 		gnavamarino: review+
Details | Formatted Diff | Diff
Patch (2.28 KB, patch)
2021-10-08 14:27 PDT, Gabriel Nava Marino 		no flags Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2021-10-12 09:29 PDT, Gabriel Nava Marino 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2021-10-07 16:48:05 PDT 
In some cases the provisionalDocumentLoader will not be there (i.e. when DocumentLoader::startLoadingMainResource: Returning empty document"), so the returned value of WKBundleFrameCopyProvisionalURL will not be valid.
Comment 1 Gabriel Nava Marino 2021-10-07 16:57:02 PDT 
Created attachment 440556 [details]
Patch
Comment 2 youenn fablet 2021-10-08 07:52:56 PDT 
Comment on attachment 440556 [details]
Patch

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

> LayoutTests/fast/harness/testrunner-fetch-empty-document.html:4
> +      testRunner.dumpAsText();

Test is failing on Mac debug wk1 as well, this is worth checking.
I also tried the test locally on mock-wk2-debug and it is passing without the patch.
Comment 3 Gabriel Nava Marino 2021-10-08 14:21:10 PDT 
(In reply to youenn fablet from comment #2)
> Comment on attachment 440556 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440556&action=review
> 
> > LayoutTests/fast/harness/testrunner-fetch-empty-document.html:4
> > +      testRunner.dumpAsText();
> 
> Test is failing on Mac debug wk1 as well, this is worth checking.
> I also tried the test locally on mock-wk2-debug and it is passing without
> the patch.

This issue is only reproducible when the test is loaded from an http server AND the URL path is not 127.0.0.1/localhost.

Unfortunately the test runner uses 127.0.0.1 so we cannot simply move this test to the http folder under LayoutTests.
Comment 4 Gabriel Nava Marino 2021-10-08 14:27:01 PDT 
Created attachment 440672 [details]
Patch
Comment 5 youenn fablet 2021-10-12 00:46:51 PDT 
Comment on attachment 440672 [details]
Patch

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

> Tools/ChangeLog:9
> +        (WTR::InjectedBundlePage::willSendRequestForFrame):

Let's add a comment explaining the case for this check:
Add mainFrameURL check in case of loading a non localhost URL with WebKitTestRunner.
Comment 6 Gabriel Nava Marino 2021-10-12 09:29:25 PDT 
Created attachment 440948 [details]
Patch
Comment 7 youenn fablet 2021-10-12 10:24:59 PDT 
Please put cq? if you want me to cq+ it
Comment 8 Gabriel Nava Marino 2021-10-12 14:29:35 PDT 
(In reply to youenn fablet from comment #7)
> Please put cq? if you want me to cq+ it

Added! Thank you for the head's up.
Comment 9 EWS 2021-10-12 14:58:16 PDT 
Committed r284039 (242869@main): <https://commits.webkit.org/242869@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440948 [details].
Comment 10 Radar WebKit Bug Importer 2021-10-12 14:59:19 PDT 
<rdar://problem/84167947>