1821169 - ReadableStream::IteratorReturn calls ReadableStreamDefaultReaderRelease on a reader with a null mStream

Status: RESOLVED WORKSFORME

(Core :: DOM: Streams, defect, P3)

Comment 1

[Peter Van der Beken [:peterv]]

See bug 1818630 for more information.

Looking at crash reports, it looks like we're crashing in ReadableStreamReaderGenericRelease coming from here: https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/dom/streams/ReadableStream.cpp#956 (through ReadableStreamDefaultReaderRelease). At that point the mStream member of reader looks to be null. Note that we've accessed that in https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/dom/streams/ReadableStream.cpp#949-950 (ReadableStreamCancel calls various methods on the stream that is passed in). So it seems that ReadableStreamCancel somehow causes the mStream member of reader to be nulled out. Hopefully somebody more familiar with the streams implementation can figure out how that can happen.

Comment 2

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

Bug 1818630's crash reports always have BodyStream and ~nsAutoMicroTask in the call stack, so there's a high possibility that nsAutoMicroTask breaks into the originally expected procedure and causes unexpected release. I observed this behavior when dealing with bug 1818387.

Comment 3

[Tom S. (please needinfo tschuster)]

Does this https://searchfox.org/mozilla-central/query/default?q=calls-between%3A%27ReadableStreamReaderGenericRelease%27+calls-between%3A%27ReadableStreamCancel%27+depth%3A20 help?

Comment 4

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

10 months later there's no BodyStream nor nsAutoMicroTask in the crash reports as they are removed (e.g. https://crash-stats.mozilla.org/report/index/b8e8defb-9090-44af-aeca-b47f50240112#tab-details). The graph in Comment #3 says pipe can release the reader, but in that case the pipe would be the sole owner of the reader and that would block the use of iterator. 🤔

Could the cancel callback be starting the global teardown and causing cycle collection?

Comment 5

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

Okay, my another trial for manual fuzzing failed. The only thing I learned is that all crashes somehow involve manually getting the iterator object and calling .next() and .return(), as the stack involves promise then callback that calls IteratorReturn, which doesn't happen with for-await-of.

Comment 6

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

  const stream = new ReadableStream({
    pull(c) {
        c.enqueue("foo")
        c.close();
    },
  }, { highWaterMark: 0 })
  const iter = stream[Symbol.asyncIterator]();
  iter.next();
  iter.next();
  iter.return();

This can release the reader before return(), but this way the stream closure sets mIsFinished=true and thus AsyncIterableReturnImpl::ReturnSteps returns early without calling IteratorReturn.

Comment 7

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

I wonder we can get some help from the fuzzer from the info in comment #5. Jason, do you think it's possible to get a guided fuzzing test?

Comment 8

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

The bug is linked to a topcrash signature, which matches the following criteria:

• Top 20 desktop browser crashes on beta
• Top 10 content process crashes on beta

:smaug, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Kagami, anything we could do here sooner than later?

Comment 10

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

This was rare enough and suddenly became top crash? 🤔 crash stat strongly implies all the reports are from the same machine at a single day. Perhaps there's a certain website that triggers this and a user tried it multiple times.

There's MOZ_DIAGNOSTIC_ASSERT so we can downgrade it to MOZ_ASSERT if this keeps being topcrash. For now this is stalled as we don't have a test case nor pernosco session.

Comment 11

[Andrew McCreight [:mccr8]]

I think something is messed up with the beta top crash detector. I've seen a number of things get flagged as top crashes. Maybe a new beta rolled out and the total volume was low or something? So I wouldn't worry about it.

Comment 12

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

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 13

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

Closing because no crashes reported for 12 weeks.

Comment 14

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit BugBot documentation.