Bug 203091

Created attachment 381632 [details] WIP Patch

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

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

Created attachment 381646 [details] Patch

<rdar://problem/56525333>

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?

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.

> >> 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.

> > 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.

(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.

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.

> 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.

> 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.

(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?

(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.

> 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.

(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.

Created attachment 381681 [details] Patch

Created attachment 381687 [details] Patch

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)

(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.

> 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.

Created attachment 381722 [details] Patch

(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

Comment on attachment 381722 [details] Patch Clearing flags on attachment: 381722 Committed r251495: <https://trac.webkit.org/changeset/251495>

All reviewed patches have been landed. Closing bug.