Bug 203091 – FetchRequest should not prevent entering the back/forward cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

FetchRequest should not prevent entering the back/forward cache.

[Chris Dumez]

Created attachment 381632 [details]
WIP Patch

[EWS Watchlist]

Comment on attachment 381632 [details]
WIP Patch

Attachment 381632 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13165277

New failing tests:
fast/history/page-cache-active-fetch-request.html

[EWS Watchlist]

Created attachment 381644 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Chris Dumez]

Created attachment 381646 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56525333>

[youenn fablet]

Comment on attachment 381646 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        suspended.

We probably need to handle FetchBodyOwner::blobChunk as well since it can trigger some JS execution.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:273
> +

If document is not suspended, can we just execute the task directly?
This would be faster and closer to what we are doing with events (dispatchEventWhenFeasible/Possible) and promises in RTCPeerConnection.

> LayoutTests/fast/history/page-cache-active-fetch-request.html:5
> +<script src="../../resources/js-test.js"></script>

This would be nice to have testharness.js based tests so that we would make sure other browsers b/f cache implementation is similar to ours.

> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
> +    request.blob().then(() => {

Can we call request.text() here and check the result is textData?
Can we also add a test with readableStream?

[Chris Dumez]

Comment on attachment 381646 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        suspended.
> 
> We probably need to handle FetchBodyOwner::blobChunk as well since it can trigger some JS execution.

Ok, I must have missed that one.

>> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:273
>> +
> 
> If document is not suspended, can we just execute the task directly?
> This would be faster and closer to what we are doing with events (dispatchEventWhenFeasible/Possible) and promises in RTCPeerConnection.

Ok, I will look into this.

>> LayoutTests/fast/history/page-cache-active-fetch-request.html:5
>> +<script src="../../resources/js-test.js"></script>
> 
> This would be nice to have testharness.js based tests so that we would make sure other browsers b/f cache implementation is similar to ours.

I prefer js-test.js. We do not have requirements to use testharness.js at this point. Also, back/forward caching is not standardized anyway.

>> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
>> +    request.blob().then(() => {
> 
> Can we call request.text() here and check the result is textData?
> Can we also add a test with readableStream?

How do those relate to my change? My change only impacts navigating while having a blobLoader. I am not familiar with readableStream but looking at text(), it does not seem to use a blob loader.

[youenn fablet]

> >> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
> >> +    request.blob().then(() => {
> > 
> > Can we call request.text() here and check the result is textData?
> > Can we also add a test with readableStream?
> 
> How do those relate to my change? My change only impacts navigating while
> having a blobLoader. I am not familiar with readableStream but looking at
> text(), it does not seem to use a blob loader.

blob loader is used to read the response body when the internal body is a blob.
Once response body is read, it can be used to create a blob if blob() is called, a string if text() is called and to feed a ReadableStream in case request.body is called.

It is useful to verify that the API is working as expected even if suspend/unsuspend happened in the middle, in that case that the expected data is actually given to the page after unsuspension.

[youenn fablet]

> > This would be nice to have testharness.js based tests so that we would make sure other browsers b/f cache implementation is similar to ours.
> 
> I prefer js-test.js. We do not have requirements to use testharness.js at
> this point. Also, back/forward caching is not standardized anyway.

Sure, but we should strive to standardise this, it does not seem great that an unsuspended page in Firefox would behave differently from the same page in Safari.
Tests that can run easily in other browsers CI are very valuable for that purpose.

[Chris Dumez]

(In reply to youenn fablet from comment #8)
> > >> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
> > >> +    request.blob().then(() => {
> > > 
> > > Can we call request.text() here and check the result is textData?
> > > Can we also add a test with readableStream?
> > 
> > How do those relate to my change? My change only impacts navigating while
> > having a blobLoader. I am not familiar with readableStream but looking at
> > text(), it does not seem to use a blob loader.
> 
> blob loader is used to read the response body when the internal body is a
> blob.

So the only code path that my patch is altering is when the internal body is a blob.
Other code paths may not behave correctly with regards to page cache but that would be pre-existing bugs and out of scope.

[Chris Dumez]

Comment on attachment 381646 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchRequest.cpp:-335
> -    return isActive();

This is the behavior change of my patch. And isActive() is implemented as follows:
bool isActive() const { return !!m_blobLoader; }

So having a m_blobLoader. Other promises not involving a blob loader are not in scope and likely were always buggy.

[youenn fablet]

> So the only code path that my patch is altering is when the internal body is
> a blob.

The test would be modified to something like:

request = new Request("", {"method": "POST", "body": blob });
// request.text() will create a BlobReader.
request.text().then((text) => {
   // unsuspension happened.
   assert_equals(text, textData)
   ...
});
// suspension happens.

Ditto for using request.body (and maybe calling getReader().read()) to make sure pushing chunks to the stream will not trigger JS execution while being suspended.

[youenn fablet]

> So having a m_blobLoader. Other promises not involving a blob loader are not
> in scope and likely were always buggy.

I am not sure to understand your comment.
I believe WebKit ToT does not have bugs related to FetchRequest and page cache.

Calling request.blob() or request.text() will both create a BlobLoader if the internal body of the request is a blob.

[Chris Dumez]

(In reply to youenn fablet from comment #8)
> > >> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
> > >> +    request.blob().then(() => {
> > > 
> > > Can we call request.text() here and check the result is textData?
> > > Can we also add a test with readableStream?
> > 
> > How do those relate to my change? My change only impacts navigating while
> > having a blobLoader. I am not familiar with readableStream but looking at
> > text(), it does not seem to use a blob loader.
> 
> blob loader is used to read the response body when the internal body is a
> blob.
> Once response body is read, it can be used to create a blob if blob() is
> called, a string if text() is called and to feed a ReadableStream in case
> request.body is called.

I was able to write an "internal blob body read as a text" test pretty easily. ReadableStream is a bit more complicated though:
    blob = new Blob([textData], { "type" : "text/plain" });
    request = new Request("", {"method": "POST", "body": blob });
    readableStream = request.body;
    shouldBeTrue("!!readableStream");

Then what do I need to do with the readableStream to get the blobLoader to start loading?

[Chris Dumez]

(In reply to youenn fablet from comment #13)
> > So having a m_blobLoader. Other promises not involving a blob loader are not
> > in scope and likely were always buggy.
> 
> I am not sure to understand your comment.
> I believe WebKit ToT does not have bugs related to FetchRequest and page
> cache.
> 
> Calling request.blob() or request.text() will both create a BlobLoader if
> the internal body of the request is a blob.

Oh no, I understood that part and am working on improving the tests. My comment merely said that my code change only impacted having an internal blob body, which is true.

[youenn fablet]

> Then what do I need to do with the readableStream to get the blobLoader to
> start loading?

I think it should start oncw the ReadableStream is created so your code should be good.
If not, request.body.getReader().read() will certainly start the blob load.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to youenn fablet from comment #8)
> > > >> LayoutTests/fast/history/page-cache-active-fetch-request.html:34
> > > >> +    request.blob().then(() => {
> > > > 
> > > > Can we call request.text() here and check the result is textData?
> > > > Can we also add a test with readableStream?
> > > 
> > > How do those relate to my change? My change only impacts navigating while
> > > having a blobLoader. I am not familiar with readableStream but looking at
> > > text(), it does not seem to use a blob loader.
> > 
> > blob loader is used to read the response body when the internal body is a
> > blob.
> > Once response body is read, it can be used to create a blob if blob() is
> > called, a string if text() is called and to feed a ReadableStream in case
> > request.body is called.
> 
> I was able to write an "internal blob body read as a text" test pretty
> easily. ReadableStream is a bit more complicated though:
>     blob = new Blob([textData], { "type" : "text/plain" });
>     request = new Request("", {"method": "POST", "body": blob });
>     readableStream = request.body;
>     shouldBeTrue("!!readableStream");
> 
> Then what do I need to do with the readableStream to get the blobLoader to
> start loading?

Ok, I was able to use:
blob = new Blob([textData], { "type" : "text/plain" });
    request = new Request("", {"method": "POST", "body": blob });
    readableStream = request.body;
    shouldBeTrue("!!readableStream");
    readableStream.getReader().read().then(() => { );

However, the promise does not get resolved after restoring from the page cache. My bet is that ReadableStream / ReadableStream reader are not page-cache aware and try to resolve promises while in the cache.

[Chris Dumez]

Created attachment 381681 [details]
Patch

[Chris Dumez]

Created attachment 381687 [details]
Patch

[youenn fablet]

Comment on attachment 381687 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:299
> +    runNetworkTaskWhenPossible([this, protectedThis = makePendingActivity(*this)]() mutable {

Can We makePendingActivity in runNetworkTaskWhenPossible in the postTask/queueTask case only.
In most cases, we do not need to ref/unref unnecessarily.

> LayoutTests/fast/history/page-cache-active-fetch-request-blobReadAsReadableStream.html:36
> +    readableStream.getReader().read().then(() => {

Can we read the stream and validate that data is correct?
The validateStreamFromString routine from LayoutTests/imported/w3c/web-platform-tests/fetch/api/resources/utils.js can be used almost as is (modulo assert_ -> shouldBe).

> LayoutTests/fast/history/page-cache-active-fetch-request-blobReadAsText.html:34
> +    request.text().then(() => {

Can we check the text promise value?
Something like: request.text().then((text) => { shouldBe(text, textData)

[Chris Dumez]

(In reply to youenn fablet from comment #20)
> Comment on attachment 381687 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381687&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:299
> > +    runNetworkTaskWhenPossible([this, protectedThis = makePendingActivity(*this)]() mutable {
> 
> Can We makePendingActivity in runNetworkTaskWhenPossible in the
> postTask/queueTask case only.
> In most cases, we do not need to ref/unref unnecessarily.
> 

Ok.

> > LayoutTests/fast/history/page-cache-active-fetch-request-blobReadAsReadableStream.html:36
> > +    readableStream.getReader().read().then(() => {
> 
> Can we read the stream and validate that data is correct?

I cannot. As mentioned earlier, the promise that does get resolved in the context of the test. It seems that ReadableStream (or reader) is trying to resolve promises while in the PageCache, which seems like a pre-existing bug.

> The validateStreamFromString routine from
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/resources/utils.js can
> be used almost as is (modulo assert_ -> shouldBe).
> 
> > LayoutTests/fast/history/page-cache-active-fetch-request-blobReadAsText.html:34
> > +    request.text().then(() => {
> 
> Can we check the text promise value?
> Something like: request.text().then((text) => { shouldBe(text, textData)

Ok.

[youenn fablet]

> I cannot. As mentioned earlier, the promise that does get resolved in the
> context of the test. It seems that ReadableStream (or reader) is trying to
> resolve promises while in the PageCache, which seems like a pre-existing bug.

OK, let's file a bug then.
I can try to look at it Friday.

[Chris Dumez]

Created attachment 381722 [details]
Patch

[Chris Dumez]

(In reply to youenn fablet from comment #22)
> > I cannot. As mentioned earlier, the promise that does get resolved in the
> > context of the test. It seems that ReadableStream (or reader) is trying to
> > resolve promises while in the PageCache, which seems like a pre-existing bug.
> 
> OK, let's file a bug then.
> I can try to look at it Friday.

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

[Chris Dumez]

Comment on attachment 381722 [details]
Patch

Clearing flags on attachment: 381722

Committed r251495: <https://trac.webkit.org/changeset/251495>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.