1571686 - Crash in [@ js::UnwrapAndDowncastObject<T>]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-ff4381ae-64de-4919-8486-0fb970190806.

Seen while looking at Windows 69 beta crash data: https://mzl.la/2ZD5CDS. Please rebucket if this is the wrong component. 2 crashes while 69 was in nightly, and then started being visible in beta in 69.0b3.

(100.0% in signature vs 20.65% overall) reason = EXCEPTION_ACCESS_VIOLATION_READ
(27.52% in signature vs 06.98% overall) Module "cabinet.dll" = true [91.11% vs 06.85% if startup_crash = null]
(100.0% in signature vs 25.17% overall) address = 0x0
(65.77% in signature vs 01.28% overall) useragent_locale = zh-CN [80.00% vs 01.47% if startup_crash = null]
(36.24% in signature vs 64.69% overall) ipc_fatal_error_protocol = null [100.0% vs 34.80% if process_type = content]
(32.89% in signature vs 12.04% overall) Module "mfperfhelper.dll" = true [75.56% vs 13.86% if startup_crash = null]
(65.77% in signature vs 15.63% overall) Module "libEGL.dll" = true
(33.56% in signature vs 73.80% overall) Addon "[e-mail]" = true
(100.0% in signature vs 68.63% overall) Module "softokn3.dll" = true
(23.49% in signature vs 04.04% overall) Module "MSVP9DEC.dll" = true [53.33% vs 08.15% if startup_crash = null]
(31.54% in signature vs 67.27% overall) Addon "[e-mail]" = true

Top 10 frames of crashing thread:

0 xul.dll js::UnwrapAndDowncastObject<js::ReadableStream> js/src/vm/Compartment-inl.h:228
1 xul.dll JS::ReadableStreamUpdateDataAvailableFromSource js/src/builtin/Stream.cpp:4672
2 xul.dll nsresult mozilla::dom::BodyStream::OnInputStreamReady dom/base/BodyStream.cpp:374
3 xul.dll nsresult nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:91
4 xul.dll bool mozilla::dom::`anonymous namespace'::ExternalRunnableWrapper::WorkerRun dom/workers/WorkerPrivate.cpp:175
5 xul.dll mozilla::dom::WorkerRunnable::Run dom/workers/WorkerRunnable.cpp:363
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
8 xul.dll mozilla::dom::WorkerPrivate::DoRunLoop dom/workers/WorkerPrivate.cpp:2815
9 xul.dll nsresult mozilla::dom::workerinternals::`anonymous namespace'::WorkerThreadPrimaryRunnable::Run dom/workers/RuntimeService.cpp:2336

Comment 1

[Andrew Overholt [:overholt]]

Jason, does this stack ring any bells for you?

Comment 2

[Jason Orendorff [:jorendorff]]

It looks like DOM is passing a null pointer to JS::ReadableStreamUpdateDataAvailableFromSource.

Baku, any ideas?

Comment 3

[Andrea Marchesini [:baku]]

Baku, any ideas?

BodyStreamHolders (Response and BlobBodyStreamHolder) do not nullify the stream by themselves. NullifyStream() is called only by BodyStream when its state has been already set to 'eClosed':
https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/base/BodyStream.cpp#484,502

In OnInputStream we check the state before using the stream:
https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/base/BodyStream.cpp#331-334,347

If OnInputStream is executed, we are definitely coming from requestData() or from writeIntoReadRequestBuffer() and our state will be eReading or eChecking. We have an assertion about that. In both methods, the stream is active.

Could it be that NewReadableExternalSourceStreamObject() returns nullptr but then, it calls requestData()? Here:
https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/base/BodyStream.cpp#109-114

Comment 4

[Marcia Knous [:marcia]]

#18 top crash in 69.0b12.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Andrea Marchesini [:baku] from comment #3)

Could it be that NewReadableExternalSourceStreamObject() returns nullptr but then, it calls requestData()? Here:
https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/base/BodyStream.cpp#109-114

Hmm. Good question, but I don't think that is possible.

The stack when we commit to calling requestData looks like this:

• JS::NewReadableExternalSourceStreamObject, line 4561
• ↪ js::ReadableStream::createExternalSourceStream, line 492
• ↪ SetUpExternalReadableByteStreamController, line 3632
• ↪ JS::AddPromiseReactions, line 3969
• ↪ CallOriginalPromiseThenImpl, line 3948
• ↪ js::OriginalPromiseThen, line 3795
• ↪ PerformPromiseThen, line 4714
• ↪ PerformPromiseThenWithReaction, line 4793
• ↪ EnqueuePromiseReactionJob, line 1104
• ↪ JSRuntime::enqueuePromiseJob, line 622

(The actual call to requestData happens a bit later, when the promise job fires.)

In the browser, that calls CycleCollectedJSContext::enqueuePromiseJob(), which is infallible; and every call in this stack is the last fallible step. From there, success is guaranteed.

Do you think it's possible BodyStream::Create can be called more than once, passing the same BodyStreamHolder? Maybe from here?

In any case, if we can't solve it right now, we should replace the MOZ_DIAGNOSTIC_ASSERT here with a release assert, and add more asserts so that the crash reports will tell us whether the stream was nullified or never fully initialized.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1571686 - Check if the ReadableStream object exists before using it in BodyStream, r?smaug

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecdf8f05db28
Check if the ReadableStream object exists before using it in BodyStream, r=smaug

Comment 9

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 10

[Cristina Coroiu [:ccoroiu]]

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

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9090506 [details]
Bug 1571686 - Check if the ReadableStream object exists before using it in BodyStream, r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: A crash can occur.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): To be honest, we don't know why we crash. We know that there is a null pointer and this patch checks that. This is safe and it fixes the crash. We have a follow up to dig more into this issue.
• String changes made/needed: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9090506 [details]
Bug 1571686 - Check if the ReadableStream object exists before using it in BodyStream, r?smaug

Approved for 70.0b4 so we can get some data ASAP before considering this for dot release ride-along.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5aba81ecdc38

Comment 14

[Marcia Knous [:marcia]]

Although this crash seems to be Windows specific, there is a macOS crash signature that seems a bit similar: https://bit.ly/2kA0XU5. If it isn't related to this bug, I can file a new one.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 9090506 [details]
Bug 1571686 - Check if the ReadableStream object exists before using it in BodyStream, r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: A crash can occur.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix consists of a nullptr check. We want to understand why the stream point is null, but in a follow-up.
• String changes made/needed:

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9090506 [details]
Bug 1571686 - Check if the ReadableStream object exists before using it in BodyStream, r?smaug

Adds a null check to work around an Fx69 topcrash. Approved for 69.0.1.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/5983d7d98ec2