1941210 - `new Response(new ReadableStream()).text()` can be aborted too soon by cycle collection

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

var i = 0;
await new Response(new ReadableStream({
  pull(c) {
    if (i >= 1000) {
      c.close();
      return;
    }
    ++i;
    c.enqueue(new Uint8Array([0]))
    TestUtils.gc(); // enabled with dom.testing.testutils.enabled=true
  }
})).arrayBuffer();

Expected: An ArrayBuffer with size 1000
Actual on Nightly: An ArrayBuffer with size 1

This is a kinda edge case where the stream is owned only by itself and nobody else. I don't know why anyone would do this rather than purely for testing. (Thus S4 while P2 because it's regression.)

Nothing is strongly holding FetchStreamReader while waiting for OutputStreamHolder. Might want to have a temporary strong reference just as we have mAsyncWaitWorkerRef right now. (And thanks to mAsyncWaitWorkerRef this works fine on workers)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1939295

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

Looks like this regression could be covered but was hidden by wpt-sync's aggressive test result additions... https://searchfox.org/mozilla-central/rev/9f9fa5d2cb72b12ac7e168b7f6ee9820f63291e9/testing/web-platform/meta/service-workers/service-worker/fetch-event-respond-with-readable-stream.https.html.ini

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1941210 - Strongly grab FetchStreamReader while waiting for writing r=smaug,jesup

Nothing strongly grabs ReadableStream nor FetchStreamReader while waiting for nsIAsyncOutputStream to respond. mAsyncWaitReader should now strongly grab the reader until the output stream responds.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4e3877220b1
Strongly grab FetchStreamReader while waiting for writing r=smaug

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/50077 for changes under testing/web-platform/tests

Comment 6

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/f4e3877220b1

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:saschanaz, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1941210 - Strongly grab FetchStreamReader while waiting for writing r=smaug,jesup

Nothing strongly grabs ReadableStream nor FetchStreamReader while waiting for nsIAsyncOutputStream to respond. mAsyncWaitReader should now strongly grab the reader until the output stream responds.

Original Revision: https://phabricator.services.mozilla.com/D234031

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: In an edge case one may get incomplete result when consuming a stream
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: Minimal changes
• String changes made/needed: No
• Is Android affected?: yes

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/56a8cf234df0

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

We got a real world report of this regression in bug 1942573.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1941210 - Strongly grab FetchStreamReader while waiting for writing r=smaug,jesup

Nothing strongly grabs ReadableStream nor FetchStreamReader while waiting for nsIAsyncOutputStream to respond. mAsyncWaitReader should now strongly grab the reader until the output stream responds.

Original Revision: https://phabricator.services.mozilla.com/D234031

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1941210 - Strongly grab FetchStreamReader while waiting for writing r=smaug,jesup

Nothing strongly grabs ReadableStream nor FetchStreamReader while waiting for nsIAsyncOutputStream to respond. mAsyncWaitReader should now strongly grab the reader until the output stream responds.

Original Revision: https://phabricator.services.mozilla.com/D234031

Comment 15

[Pascal Chevrel:pascalc]

Kagami, if we need to uplift a patch to all of our branches, I think the severity of this bug shouldn't be an S4.

Is the new revision you created just a copy of the patch that was already uplifted to beta? (seems to me at first sight), if so I can manually graft it to mozilla-release and carry over the existing reviewer.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Let's make it S3. Yes, it's just the same patch, cleanly grafted.

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9460495 [details]
Bug 1941210 - Strongly grab FetchStreamReader while waiting for writing r=smaug,jesup

Functional regression in 134, approved for 134.0.2, thanks.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/33bb8362cc38

Comment 19

[Pascal Chevrel:pascalc]

Kagami, could you provide a release note summarizing the issue please?
You can look at past release notes as a template: https://www.mozilla.org/en-US/firefox/134.0.1/releasenotes/
Thanks

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

How about "Fixed an issue where data consumption from service workers may unexpectedly halt."

While technically this doesn't only affect service worker, for now that's the only reported real world case.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/daf7073c4a54