1539208 - Crash in [@ mozilla::dom::FetchDriver::OnDataAvailable]

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-e2a901ca-d898-4dc5-bbe9-6d95f0190326.

Seen while looking at Fennec release crash stats, but visible on other branches as well: https://bit.ly/2TwONXr. This was visible in 65/65.0.1 with over 3K in crashes. #19 overall in the current 66 release.

Comments mentioning scrolling and zooming in on openstreet maps.

(40.26% in signature vs 00.46% overall) useragent_locale = fi

Top 10 frames of crashing thread:

0 libxul.so mozilla::dom::FetchDriver::OnDataAvailable dom/fetch/InternalResponse.h:71
1 libxul.so nsCORSListenerProxy::OnDataAvailable netwerk/protocol/http/nsCORSListenerProxy.cpp:636
2 libxul.so mozilla::net::InterceptedHttpChannel::OnDataAvailable netwerk/protocol/http/InterceptedHttpChannel.cpp:1080
3 libxul.so nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:554
4 libxul.so nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:91
5 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1180
6 libxul.so NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:482
7 libxul.so mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:88
8 libxul.so MessageLoop::Run ipc/chromium/src/base/message_loop.cc:315
9 libxul.so nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137

Comment 1

[Honza Bambas (:mayhemer)]

Seems like we are crashing here:
https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/dom/fetch/FetchDriver.cpp#1184

mResponse is null.

The problem could potentially be here:
https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/dom/fetch/FetchDriver.cpp#1051

the |rv| is NS_OK, but it should be an explicit error.

Regression from:
https://searchfox.org/mozilla-central/commit/a1e22fa9e7ead38dc0e0cef845f358b97a34a508

Comment 2

[Andrew Overholt [:overholt]]

Tom, can you take a look here?

Comment 3

[Tom Tung [:tt, :ttung]]

Sure, I think Honza's analysis is right. It returns NS_OK unexpectedly for StartRequest() so that OnDataAvailable() is possible to be called with a dangling mResponse.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1539208 - P2 - Use RandomNum while failing to get a random number from RandomGenerator;

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1539208 - P1 - Return an error when the Reponse object is null'd;

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cec67adbdbb6
P1 - Return an error when the Reponse object is null'd; r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/7b17d77256be
P2 - Use RandomNum while failing to get a random number from RandomGenerator; r=baku

Comment 7

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/cec67adbdbb6
https://hg.mozilla.org/mozilla-central/rev/7b17d77256be

Comment 8

[Andreea Pavel [:apavel]]

Backed out for causing bug 1540569

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-android-em-7.0-x86_64%2Fdebug-jsreftest-e10s-2%2Cr-e10s%28j2%29&revision=7b17d77256be8a810d75865b422a49b4adca7a90&selectedJob=237264210

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=237264210&repo=autoland&lineNumber=15600

Backout: https://hg.mozilla.org/mozilla-central/rev/12014960188da2a6c78f281b072f9bb9f69ec8ef

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b7207f23c2f
P2 - Use RandomNum while failing to get a random number from RandomGenerator;
https://hg.mozilla.org/integration/autoland/rev/107061d96dd2
P1 - Return an error when the Reponse object is null'd;

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8b7207f23c2f
https://hg.mozilla.org/mozilla-central/rev/107061d96dd2

Comment 11

[Pascal Chevrel:pascalc]

Tom, do you estimate that this would be safe to uplift to 67? (Not a lot of crashes on beta 67 but it wasn't a lot either in 66 beta while it is noticeable on 66 release) Thanks

Comment 12

[Tom Tung [:tt, :ttung]]

This should be safe because it only corrects the behavior by returning a proper error (it returned an NS_OK for the failure case) and tries another way to generate a random number by another API.

I'll send an uplift request to Beta.

Comment 13

[Tom Tung [:tt, :ttung]]

Comment on attachment 9054198 [details]
Bug 1539208 - P1 - Return an error when the Reponse object is null'd;

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1290481
• User impact if declined: It would crash if it failed to get a random number service because the code returned an NS_OK for that case.
• 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): P1 corrects the returning nsresult so it shouldn't have any risky in theory.
  P2 generates a random number by another API while the original service fails so it also shouldn't have any risky in theory.
• String changes made/needed:

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9054198 [details]
Bug 1539208 - P1 - Return an error when the Reponse object is null'd;

Crash fix, low risk patch and the volume on 66 release for this signature is medium. Uplift approved for 67 beta 9, thanks!

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6168e6cc5761
https://hg.mozilla.org/releases/mozilla-beta/rev/4b57a43c92b9

Comment 16

[[:philipp]]

hi, crash reports are continuing to come in from builds containing the patch unfortunately.
as per the user at https://support.mozilla.org/questions/1256474 visiting www.sciencespots.com on fennec reproducibly crashes the browser (which i can confirm on 67.0b9).

Comment 17

[Tom Tung [:tt, :ttung]]

Will look into this. Thanks!

Comment 18

[Tom Tung [:tt, :ttung]]

I did a stupid check on [1]. However, in Nightly, the fact we didn't hit this nullptr does prove the crash is not coming from Bug 1290481.

We also have an assertion for mResponse inside the ShouldCheckSRI() [2].

I wonder if the problem is something else.

[1] https://searchfox.org/mozilla-beta/rev/22be965751d52a56f4e6920d58283152e0d8bec0/dom/fetch/FetchDriver.cpp#1050
[2] https://searchfox.org/mozilla-beta/rev/22be965751d52a56f4e6920d58283152e0d8bec0/dom/fetch/FetchDriver.cpp#1056

Comment 19

[Tom Tung [:tt, :ttung]]

Eden says he is recently working on issues on Android and is happy to look into this.

Comment 20

[Eden Chuang[:edenchuang]]

Analyzing the case on an emulator, FetchDriver::OnStartRequest() exits in

https://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#814

In the case, FetchDriver::mChannel is nullptr, and mResponse is not created in OnStartRequest. Then when we receive the OnDataAvailable, it accesses a nullptr (mResponse->Type()).

https://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#1184

Now there are two possible problems.

1. mChannel should not be nullptr at the moment.
2. In theory, if FetchDriver::OnStartRequest() returns fail, FetchDriver::OnDataAvailable should not be called. But it is called in this case.

Comment 21

[Eden Chuang[:edenchuang]]

InterceptedHttpChannel::OnStartReqeust() doesn't propagate the error from its mListener. It always returns NS_OK, even mListener->OnStartRequest() failed.

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedHttpChannel.cpp#1023

That's the reason why the nsInputStreamPump's state doesn't change as our expected to STATE_STOP.

Comment 22

[Honza Bambas (:mayhemer)]

Yes, that is a huge bug, good catch!

Comment 23

[Eden Chuang[:edenchuang]]

Attachment: Propagate the result of mListener in InterceptedHttpChannel::OnStartRequest and ::OnStopRequest

1. Propagate the mListener's result in InterceptedHttpChannel::OnStartRequest()
   and ::OnStopRequest().

2. remove unnecessary assertion in FetchDriver::OnStartRequest().

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c94a6d04f305
Propagate the result of mListener in InterceptedHttpChannel::OnStartRequest and ::OnStopRequest r=perry,ttung,kershaw

Comment 25

[Cristian Brindusan [:cbrindusan]]

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

Comment 26

[Eden Chuang[:edenchuang]]

Pascal, need we uplift the patch to the beta?

Comment 27

[Pascal Chevrel:pascalc]

(In reply to Eden Chuang[:edenchuang] from comment #26)

Pascal, need we uplift the patch to the beta?

Could you request an uplift and give your evaluation of the risks? I'll look at it tomorrow, thanks!

Comment 28

[Eden Chuang[:edenchuang]]

Comment on attachment 9060070 [details]
Propagate the result of mListener in InterceptedHttpChannel::OnStartRequest and ::OnStopRequest

Beta/Release Uplift Approval Request

• User impact if declined: Crash when using ServiceWorker on Android.
• 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): Not risky. This patch just propagates the nsIStreamListener::OnStartRequest/OnStopRequest results to nsInputStreamPump to make nsInputStreamPump change the state correctly.
• String changes made/needed: No

Comment 29

[Pascal Chevrel:pascalc]

Comment on attachment 9060070 [details]
Propagate the result of mListener in InterceptedHttpChannel::OnStartRequest and ::OnStopRequest

Low risk crash fix for Android, uplift approved for 67 beta 15, thanks.

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/224c70ce5eff