Crash in mozilla::dom::FetchDriver::HttpFetch

VERIFIED FIXED in Firefox -esr60

Status

()

defect
P2
critical
VERIFIED FIXED
8 months ago
5 months ago

People

(Reporter: philipp, Assigned: junior)

Tracking

({crash})

64 Branch
mozilla65
Points:
---

Firefox Tracking Flags

(firefox-esr6065+ verified, firefox63 wontfix, firefox64 wontfix, firefox65 verified)

Details

(Whiteboard: [necko-triaged], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-387eb743-0cc2-4dde-8520-9cbbf0181108.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsresult mozilla::dom::FetchDriver::HttpFetch dom/fetch/FetchDriver.cpp:730
1 xul.dll nsresult mozilla::dom::FetchDriver::Fetch dom/fetch/FetchDriver.cpp:429
2 xul.dll mozilla::dom::FetchRequest dom/fetch/Fetch.cpp:563
3 xul.dll nsGlobalWindowInner::Fetch dom/base/nsGlobalWindowInner.cpp:3734
4 xul.dll static bool mozilla::dom::Window_Binding::fetch dom/bindings/WindowBinding.cpp:16369
5 xul.dll static bool mozilla::dom::Window_Binding::fetch_promiseWrapper dom/bindings/WindowBinding.cpp:16385
6 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises> dom/bindings/BindingUtils.cpp:3315
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:560
8 xul.dll static bool InternalCall js/src/vm/Interpreter.cpp:614
9 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3462

=============================================================

this signature has been around for a while already across all platforms.
in the 64.0b cycle the crash is looking like it's becoming a bit more frequent - overall it's still low volume though (0.15% of  content crashes in beta currently).
Component: DOM → DOM: Networking
:overholt - I've been told you can find someone to look into this
Flags: needinfo?(overholt)
a null-check should fix this
[1] kinda indicates cos is possible to be null, thus making sense to do another null-check
[1] https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/dom/fetch/FetchDriver.cpp#607
Flags: needinfo?(overholt)
Assignee: nobody → juhsu
Priority: -- → P2
Whiteboard: [necko-triaged]
Attachment #9026247 - Attachment description: Bug 1505844 - nullcheck to avoid crash → Bug 1505844 - nullcheck of non-ClassOfService channel in FetchDriver to avoid crash
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b8cf5fe84e5
nullcheck of non-ClassOfService channel in FetchDriver to avoid crash r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/2b8cf5fe84e5
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify-

this crash signature is spiking up on pre-65 builds (2.3k crash reports yesterday), comments are pointing towards reuters.com where probably some server-side change is starting to cause this issue since two days ago.

should we consider taking the simple patch to esr as well under these circumstances?

Flags: needinfo?(ryanvm)

This is currently the #2 overall top content process crash (#1 ignoring OOM|small) on 60.5.1esr and it's a trivial null check. Patch appears to have been effective on Fx65. Makes sense to me to take this on ESR60.

Please request approval on this when you get a chance, Junior.

Flags: needinfo?(ryanvm) → needinfo?(juhsu)

Comment on attachment 9026247 [details]
Bug 1505844 - nullcheck of non-ClassOfService channel in FetchDriver to avoid crash

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

high crash rate (#2 overall top content process crash)

User impact if declined

could crash in content process

Fix Landed on Version

65

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

simple null-check

String or UUID changes made by this patch

no

Flags: needinfo?(juhsu)
Attachment #9026247 - Flags: approval-mozilla-esr60?

hi mike, could we also partly deal with this as a Web Compatibility issue and try to reach out to reuters to convince them to look into this or roll back whatever caused this spike now (in case they haven't noticed on their own by now)? 2k crash reports per day is a fairly high number considering our current release is no longer affected...

Flags: needinfo?(miket)

Happy to attempt outreach, but the trick will be to have something concrete to ask them. Do we have any idea what is triggering the crash? Is it a server response, or something on the client?

(given the stack, presumably something calling window.fetch?)

Flags: needinfo?(miket) → needinfo?(madperson)

unfortunately i'm not sure about the technical circumstances that would trigger this (redirecting to junior) - the reuters issue started showing up in our socorro stats on 2019-02-18 around 17:30 UTC though...

Flags: needinfo?(madperson) → needinfo?(juhsu)

I attach the debugger and find they fetched a data:// URI

Flags: needinfo?(juhsu)

Thanks! I'll find someone to get in touch with and update here.

Flags: needinfo?(miket)

Comment on attachment 9026247 [details]
Bug 1505844 - nullcheck of non-ClassOfService channel in FetchDriver to avoid crash

adding null check, fixes top crash, approved for 60esr.

This might go in 60.5.2, trying to figure out if we can spin that quickly to stop the bleeding.

Attachment #9026247 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Flipping qe-verify since we seem to have a way to reproduce now.

Flags: qe-verify- → qe-verify+

The fetch in question happens in 3rd party code:

https://www.dianomi.com/js/lazyload.10.20.1.min.js:

! function(t) {
    a && s ? fetch("data:image/webp;base64,UklGRh4AAABXRUJQVlA4TBEAAAAvAAAAAAfQ//73v/+BiOh/AAA=").then(function(e) {
        return e && void 0 !== e.blob ? e.blob() : t(!1)
    }).then(function(e) {
        return t(!!window.createImageBitmap(e))
    }) : t(!1)
}

This seems like some kind of marketing/advertising script?

Reproduced the crash using comment 19 STR on Version 60.5.1esr 20190211182645 and on 63.0 20181018182531.

Verified that the crash doesn't happen on 60.5.2esr 20190221164337 and 65.0.1 20190211233335 on Windows 10 x64 / Osx 10.13.6, hence updating the status flags accordingly.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

(In reply to Mike Taylor [:miketaylr] from comment #20)

The fetch in question happens in 3rd party code:

https://www.dianomi.com/js/lazyload.10.20.1.min.js:

! function(t) {
    a && s ? fetch("data:image/webp;base64,UklGRh4AAABXRUJQVlA4TBEAAAAvAAAAAAfQ//73v/+BiOh/AAA=").then(function(e) {
        return e && void 0 !== e.blob ? e.blob() : t(!1)
    }).then(function(e) {
        return t(!!window.createImageBitmap(e))
    }) : t(!1)
}

This seems like some kind of marketing/advertising script?

Actually... a feature detection script for webp support.

https://davidwalsh.name/detect-webp

Crashes seem way down, so that's good.

Flags: needinfo?(miket)
You need to log in before you can comment on or make changes to this bug.