Bug 198469 - Memory-cached main resources continue to load after the client decides a content policy of PolicyAction::Download
Summary: Memory-cached main resources continue to load after the client decides a cont...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-02 15:47 PDT by Andy Estes
Modified: 2019-06-03 12:39 PDT (History)
11 users (show)

See Also:


Attachments
Patch (12.23 KB, patch)
2019-06-02 15:59 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.86 MB, application/zip)
2019-06-02 17:15 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews211 for win-future (13.96 MB, application/zip)
2019-06-02 19:49 PDT, EWS Watchlist
no flags Details
Patch (13.04 KB, patch)
2019-06-03 11:09 PDT, Andy Estes
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.05 MB, application/zip)
2019-06-03 11:58 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2019-06-02 15:47:38 PDT
Memory-cached main resources continue to load after the client decides a content policy of PollicyAction::Download
Comment 1 Andy Estes 2019-06-02 15:47:59 PDT
rdar://problem/50512713
Comment 2 Andy Estes 2019-06-02 15:59:29 PDT
Created attachment 371167 [details]
Patch
Comment 3 EWS Watchlist 2019-06-02 17:15:48 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-06-02 17:15:49 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-06-02 19:49:52 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-06-02 19:49:54 PDT Comment hidden (obsolete)
Comment 7 youenn fablet 2019-06-03 08:57:42 PDT
Comment on attachment 371167 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:953
> +        ASSERT(m_mainResource);

Is this assert needed given we check for m_mainResource at the beginning of the Download case block?
Comment 8 youenn fablet 2019-06-03 09:01:20 PDT
As a side note, there is the case of substitute resource loads where we call mainReceivedError() in case of PolicyAction::Download. I wonder whether we should have consistent calls there.
Comment 9 Andy Estes 2019-06-03 09:38:19 PDT
(In reply to youenn fablet from comment #8)
> As a side note, there is the case of substitute resource loads where we call
> mainReceivedError() in case of PolicyAction::Download. I wonder whether we
> should have consistent calls there.

stopLoadingForPolicyChange() calls mainReceivedError(), so I think we're consistent in that sense. It uses a different error code than in the call you cited ("interrupted for policy change" vs. "cannot show URL"), but I wanted to match the error code used in the call to ResourceLoader::didFail.
Comment 10 Andy Estes 2019-06-03 10:52:38 PDT
(In reply to youenn fablet from comment #7)
> Comment on attachment 371167 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371167&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:953
> > +        ASSERT(m_mainResource);
> 
> Is this assert needed given we check for m_mainResource at the beginning of
> the Download case block?

I was curious to see if it could ever go missing after that check at the beginning, but I don't think it's needed. Will remove.
Comment 11 Andy Estes 2019-06-03 11:09:17 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-06-03 11:58:57 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-06-03 11:58:59 PDT Comment hidden (obsolete)
Comment 14 Andy Estes 2019-06-03 12:39:08 PDT
Committed r246043: <https://trac.webkit.org/changeset/246043>