Bug 183297 - imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html times out with async policy delegates
Summary: imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-a-ele...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 180568
  Show dependency treegraph
 
Reported: 2018-03-02 11:57 PST by Ali Juma
Modified: 2019-10-02 06:11 PDT (History)
10 users (show)

See Also:


Attachments
WIP patch (5.79 KB, patch)
2018-03-02 12:48 PST, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.25 MB, application/zip)
2018-03-02 13:50 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.16 MB, application/zip)
2018-03-02 14:17 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.92 MB, application/zip)
2018-03-02 14:23 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.06 MB, application/zip)
2018-03-02 14:24 PST, EWS Watchlist
no flags Details
WIP patch (8.93 KB, patch)
2018-03-02 15:14 PST, Ali Juma
no flags Details | Formatted Diff | Diff
WIP patch (8.93 KB, patch)
2018-03-02 15:38 PST, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.09 MB, application/zip)
2018-03-02 21:16 PST, EWS Watchlist
no flags Details
Patch (9.88 KB, patch)
2018-03-05 09:05 PST, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-03-02 11:57:53 PST
This test times out because WebFrameLoaderClient::dispatchDidFinishLoad() never fires for the main frame.

The problem is that the last time that checkLoadCompleteForThisFrame is called for the main frame, the subframe is still waiting for a policy decision so still has a policy loader. So isLoadingInAPISense is still true for the main frame. When the subframe policy decision is received, it's "Download", which means that continueLoadAfterNavigationPolicy gets called with shouldContinue==false. So then the policy loader gets cleared, but nothing triggers another checkLoadCompleteForThisFrame on the main frame. A possible fix is to call checkLoadComplete after clearing the policy loader in the !canContinue case in continueLoadAfterNavigationPolicy.
Comment 1 Ali Juma 2018-03-02 12:48:03 PST
Created attachment 334917 [details]
WIP patch
Comment 2 EWS Watchlist 2018-03-02 13:50:22 PST Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-03-02 13:50:23 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-03-02 14:17:36 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-03-02 14:17:38 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-03-02 14:23:15 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-03-02 14:23:17 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-03-02 14:24:05 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-03-02 14:24:16 PST Comment hidden (obsolete)
Comment 10 Ali Juma 2018-03-02 15:14:12 PST
Created attachment 334934 [details]
WIP patch
Comment 11 Ali Juma 2018-03-02 15:38:22 PST
Created attachment 334936 [details]
WIP patch
Comment 12 Chris Dumez 2018-03-02 17:10:28 PST
Comment on attachment 334936 [details]
WIP patch

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

> LayoutTests/platform/ios-wk2/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')

This does not look OK.

> LayoutTests/platform/mac-wk1/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')

ditto.
Comment 13 EWS Watchlist 2018-03-02 21:16:38 PST
Comment on attachment 334936 [details]
WIP patch

Attachment 334936 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6747062

New failing tests:
http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html
http/wpt/html/browsers/history/the-location-interface/location-protocol-setter-non-broken-async-delegate.html
Comment 14 EWS Watchlist 2018-03-02 21:16:50 PST
Created attachment 334949 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 15 Ali Juma 2018-03-05 09:05:42 PST
Created attachment 335003 [details]
Patch
Comment 16 Ali Juma 2018-03-05 09:06:03 PST
(In reply to Chris Dumez from comment #12)
> Comment on attachment 334936 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334936&action=review
> 
> > LayoutTests/platform/ios-wk2/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> > +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')
> 
> This does not look OK.
> 
> > LayoutTests/platform/mac-wk1/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> > +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')
> 
> ditto.

The existing test has these errors in its expected output for mac-wk1 and ios-wk2 as well. These errors are because the 'download' attribute isn't supported on WK1 or on iOS (see bug 156069 for WK1 and bug 167341 for iOS), so the click ends up navigating. I verified that I get the same console error on WK2 without my change if I remove the 'download' attribute from the test iframe (and also get the same error in Chrome and Firefox if I remove the 'download' attribute).

Would it be better to skip these tests instead?
Comment 17 Chris Dumez 2018-03-05 09:14:31 PST
(In reply to Ali Juma from comment #16)
> (In reply to Chris Dumez from comment #12)
> > Comment on attachment 334936 [details]
> > WIP patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=334936&action=review
> > 
> > > LayoutTests/platform/ios-wk2/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> > > +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')
> > 
> > This does not look OK.
> > 
> > > LayoutTests/platform/mac-wk1/http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt:1
> > > +CONSOLE MESSAGE: line 21: TypeError: null is not an object (evaluating 'errorFrame.contentDocument.querySelector("#error-url").click')
> > 
> > ditto.
> 
> The existing test has these errors in its expected output for mac-wk1 and
> ios-wk2 as well. These errors are because the 'download' attribute isn't
> supported on WK1 or on iOS (see bug 156069 for WK1 and bug 167341 for iOS),
> so the click ends up navigating. I verified that I get the same console
> error on WK2 without my change if I remove the 'download' attribute from the
> test iframe (and also get the same error in Chrome and Firefox if I remove
> the 'download' attribute).
> 
> Would it be better to skip these tests instead?

Oh, I see that now. Please disregard my comment then.
Comment 18 Chris Dumez 2018-03-05 10:15:53 PST
Comment on attachment 335003 [details]
Patch

Clearing flags on attachment: 335003

Committed r229286: <https://trac.webkit.org/changeset/229286>
Comment 19 Chris Dumez 2018-03-05 10:15:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Chris Dumez 2018-03-05 10:16:30 PST
Comment on attachment 335003 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3162
> +        checkLoadComplete();

I think we really want checkCompleted() here instead. I'll follow up in https://bugs.webkit.org/show_bug.cgi?id=183337.
Comment 21 Radar WebKit Bug Importer 2018-03-05 10:21:05 PST
<rdar://problem/38144334>
Comment 22 Becky Spencer 2018-07-09 04:48:04 PDT
Good news It applies to everyone to use with me. Please do what you are doing. More and more communication is a new way to achieve development and development. Facilitate community communication. Hundreds of machines and images do not easily understand. We have submitted papers, letters and emails for all important information about cell phones. The university university provides communication facilities and all information. try [Bulk SMS](https://www.experttexting.com/) services for your future!
Comment 23 Brandphic 2019-10-02 06:11:37 PDT
Great offer. Get your custom logo design & website design by worlds best designers, we don't make logo, we design to make your business a brand. 100% refund guarantee if not satisfied. 
https://www.brandphic.com/