1818630 - Crash in [@ mozilla::dom::ReadableStreamReaderGenericRelease]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

186 crashes from 129 installations of Firefox 110.0, version 109.0 has no reports with this signature.

Is this a regression from bug 1809673?

[Tracking Requested - why for this release]: medium volume crash introduced in Firefox 110.0

Crash report: https://crash-stats.mozilla.org/report/index/664ed8fa-e617-4409-b9d9-64b4a0230223

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::RefPtrTraits<mozilla::dom::ReadableStream>::AddRef  mfbt/RefPtr.h:49
0  xul.dll  RefPtr<mozilla::dom::ReadableStream>::ConstRemovingRefPtrTraits<mozilla::dom::ReadableStream>::AddRef  mfbt/RefPtr.h:380
0  xul.dll  RefPtr<mozilla::dom::ReadableStream>::RefPtr  mfbt/RefPtr.h:109
0  xul.dll  mozilla::dom::ReadableStreamReaderGenericRelease  dom/streams/ReadableStreamDefaultReader.cpp:272
1  xul.dll  mozilla::dom::ReadableStreamDefaultReaderRelease  dom/streams/ReadableStreamBYOBReader.cpp:306
2  xul.dll  mozilla::dom::ReadableStream::IteratorReturn  dom/streams/ReadableStream.cpp:948
3  xul.dll  mozilla::dom::binding_detail::AsyncIterableIteratorWithReturn<mozilla::dom::ReadableStream>::GetReturnPromise  dom/bindings/IterableIterator.h:402
4  xul.dll  mozilla::dom::binding_detail::AsyncIterableReturnImpl::ReturnSteps  dom/bindings/IterableIterator.cpp:257
5  xul.dll  mozilla::dom::binding_detail::AsyncIterableReturnImpl::Return::<lambda_0>::operator const  dom/bindings/IterableIterator.cpp:290
5  xul.dll  mozilla::dom::  dom/promise/Promise-inl.h:205

Comment 1

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

The first report happened in Jan 8, before bug 1809673 landed. Looks more like bug 1734244 per the stack.

Edit: Actually in Jan 5. https://crash-stats.mozilla.org/report/index/9d73790a-4f6f-4755-a975-62f8e0230105

Comment 2

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

1 xul.dll mozilla::dom::ReadableStreamDefaultReaderRelease dom/streams/ReadableStreamBYOBReader.cpp:306

This doesn't makes sense, why is ReadableStreamDefaultReaderRelease in ReadableStreamBYOBReader.cpp?

Comment 3

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

The crash stack somehow shows ReadableStreamBYOBReaderRelease in the name of ReadableStreamDefaultReaderRelease. Can it be a compiler error? https://crash-stats.mozilla.org/report/index/2f173892-df0c-4172-9800-d938b0230224

Comment 4

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

Not all reports do that, btw... https://crash-stats.mozilla.org/report/index/fe3f0328-f70a-4324-b43b-e54750230108#tab-details

Comment 5

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

For now I have no idea how to investigate this without a repro. I found that BodyStream can cause an unexpected change to ReadableStream and that might be related here: Bug 1818387.

Comment 6

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

The bug is marked as tracked for firefox111 (beta). We have limited time to fix this, the soft freeze is in 10 days. However, the bug still isn't assigned.

:jstutte, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 7

[Jens Stutte [:jstutte]]

IIUC from how mStream is defined and used it seems, we can find it nullptr if a ReadableStreamGenericReader has been constructed but SetStream has never been called.

The stack seems to point to this assignment but cracking a dump up with VS seems to show the actual nullptr access is from the line below (ignoring the MOZ_ASSERT).

I assume we should not just assert here but also bail out/throw an error?

Comment 8

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

Maybe I should request a dump access...

Not having a stream in a reader means something very wrong happened, throwing an error should be a last resort. It means either it's not initialized at all (oh no) or it's being released twice (oh no no).

Comment 9

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

:evilpie, since you are the author of the regressor, bug 1734244, could you take a look?

For more information, please visit auto_nag documentation.

Comment 10

[Tom S. (please needinfo tschuster)]

I am really quite busy the next two weeks, I hope Kagami figures this out.

Comment 11

[Jens Stutte [:jstutte]]

Hi Tom, Kagami is on PTO the next two weeks...

Taking another look, from here it seems, mStream == nullptr can occur:

• right after initialization if SetStream was never called (unlikely due to the asserts?)
• after ReadableStreamReaderGenericRelease has been called, which seems to have quite some different call-paths, such that I would not be too surprised if we see it called twice?

Should we harden ReadableStreamReaderGenericRelease (or some of its callers) against re-entrance?

Comment 12

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

right after initialization if SetStream was never called

The init step involves SetStream, so no 🤔

Comment 13

[Tom S. (please needinfo tschuster)]

I guess we could harden ReadableStream::IteratorReturn against a null stream. That function really shouldn't be called after the iteration is already finished (and thus the stream is closed). I think that either indicates a spec issue or some problem with the implementation of iterators as provided by the WebIDL bindings. Maybe Peter has some idea?

Comment 14

[Peter Van der Beken [:peterv]]

IteratorReturn should only be called once (through GetReturnPromise). We follow the spec here: https://searchfox.org/mozilla-central/source/dom/bindings/IterableIterator.cpp#237,252,257 I don't see how it could be called multiple times for the same iterator. I don't know much about the streams implementation, so not sure what else could be going wrong, but note that there can be multiple iterators for the same object.

Comment 15

[Jens Stutte [:jstutte]]

(In reply to Peter Van der Beken [:peterv] from comment #14)

but note that there can be multiple iterators for the same object.

Could that mean that the first iterator return wins and the second looses? Must the object track usage by iterators and cleanup only when the last usage has gone?

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to Jens Stutte [:jstutte] from comment #15)

Could that mean that the first iterator return wins and the second looses? Must the object track usage by iterators and cleanup only when the last usage has gone?

I don't know what you mean by win and lose. WebIDL just defines when iterators are created and there is nothing that prevents multiple iterators being alive at the same time. It's up to individual specs to define how those iterators should function. I assume that the streams spec defines this.

Comment 17

[Tom S. (please needinfo tschuster)]

You can have only one reader per ReadableStream, so you can in theory only construct one iterator as well.

Comment 18

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

I tried to human-fuzz but failed to repro this. The only way I can imagine is to trick BodyStream to call JS involving synchronous XHR via EnqueueNative (AFAIK only piping can do this, and can unexpectedly close the stream while still reading) while the async iteration is happening, which should be fixed by bug 1818387 if it's even possible.

Edit: Sorry, bug 1818387 will fix the crash but not the sudden closing.

Comment 19

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

Picking random crash report here always shows mozilla::dom::BodyStream::OnInputStreamReady and ~nsAutoMicroTask so that must be it, but not sure how exactly 🙃

Comment 20

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

Perhaps smaug can handle the nsAutoMicroTask thing, that's probably better than trying to get a repro of the crash.

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to Tom S [:evilpie] from comment #17)

You can have only one reader per ReadableStream, so you can in theory only construct one iterator as well.

After reading the spec, it seems like creating an iterator while there is one alive already should fail, because IsReadableStreamLocked will return true (https://streams.spec.whatwg.org/#set-up-readable-stream-default-reader and https://searchfox.org/mozilla-central/source/dom/streams/ReadableStreamDefaultReader.cpp#413-418).

Comment 22

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

(In reply to Kagami pto-until-March-10 [:saschanaz] from comment #19)

Picking random crash report here always shows mozilla::dom::BodyStream::OnInputStreamReady and ~nsAutoMicroTask so that must be it, but not sure how exactly 🙃

I don't see that ~nsAutoMicroTask particularly interesting. Without nsAutoMicroTask we just might run microtasks just sooner.

Comment 23

[Peter Van der Beken [:peterv]]

(In reply to Jens Stutte [:jstutte] from comment #11)

• after ReadableStreamReaderGenericRelease has been called, which seems to have quite some different call-paths, such that I would not be too surprised if we see it called twice?

I've gone through the calls to ReadableStreamDefaultReaderRelease in ReadableStream.cpp, I think those are ok. mozilla::dom::IteratorReadRequest::CloseSteps/ErrorSteps either reject the promise or resolve it with the end of iteration-value. That should always cause the iterator to be set to the finished state. After that we shouldn't call back into the ReadableStream code anymore, we always check for the finished state before.

Comment 24

[Peter Van der Beken [:peterv]]

I don't suppose there's a way for script to get its hands on the default stream reader that is created for the async iterator? It would then for example be possible for it to call ReadableStreamDefaultReader.releaseLock.

Comment 25

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

:smaug Hey, could you set a severity on this issue? Do we want to land something for 111?

Comment 26

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

Yes, we do need to fix this somehow, I think, even in 111. This is rather frequent.
(But I'm not yet at all familiar with AsyncIterableIteratorWithReturn)

Comment 27

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

This is a reminder regarding comment #6!

The bug is marked as tracked for firefox111 (beta). We have limited time to fix this, the soft freeze is in 3 days. However, the bug still isn't assigned.

Comment 28

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

peterv, I think you said you're looking into this.

Comment 29

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

This is a reminder regarding comment #6!

The bug is marked as tracked for firefox111 (beta). We have limited time to fix this, the soft freeze is in 2 days. However, the bug still isn't assigned.

Comment 30

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

This is a reminder regarding comment #6!

The bug is marked as tracked for firefox111 (beta). We have limited time to fix this, the soft freeze is in a day. However, the bug still isn't assigned.

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: Bug 1818630 - Crash in [@ mozilla::dom::ReadableStreamReaderGenericRelease]. r?smaug!

Comment 32

[Peter Van der Beken [:peterv]]

I added a null-check, preceded by a MOZ_DIAGNOSTIC_ASSERT. That should make the crash go away on beta and release. I've filed a followup bug (bug 1821169) to figure out what is actually going on.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e95ebc865037
Crash in [@ mozilla::dom::ReadableStreamReaderGenericRelease]. r=smaug

Comment 34

[Cristina Horotan [:chorotan]]

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

Comment 35

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

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

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

For more information, please visit auto_nag documentation.

Comment 36

[Donal Meehan [:dmeehan]]

:peterv re: comment 35 - is this suitable for uplift in for a dot release ridealong?

Comment 37

[Donal Meehan [:dmeehan]]

Setting 111 to wontfix, this can ride the train with 112.