1505844 - Crash in mozilla::dom::FetchDriver::HttpFetch

Status: VERIFIED FIXED

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

Description

[[:philipp]]

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).

Comment 1

[u408661]

:overholt - I've been told you can find someone to look into this

Comment 2

[Junior [inactive]]

a null-check should fix this

Comment 3

[Junior [inactive]]

Attachment: Bug 1505844 - nullcheck of non-ClassOfService channel in FetchDriver to avoid crash

Comment 4

[Junior [inactive]]

[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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b8cf5fe84e5
nullcheck of non-ClassOfService channel in FetchDriver to avoid crash r=mayhemer

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2b8cf5fe84e5

Comment 7

[[:philipp]]

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?

Comment 8

[Ryan VanderMeulen [: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.

Comment 9

[Junior [inactive]]

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

Comment 10

[Wayne Mery (:wsmwk)]

https://www.reuters.com/article/us-usa-covington-suit/teen-in-lincoln-memorial-protest-sues-washington-post-for-250-million-idUSKCN1Q82SW reliably crashes for me using 64.0

Comment 11

[[:philipp]]

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...

Comment 12

[Mike Taylor [:miketaylr]]

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?)

Comment 13

[[:philipp]]

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...

Comment 14

[Junior [inactive]]

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

Comment 15

[Mike Taylor [:miketaylr]]

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

Comment 16

[Julien Cristau [:jcristau]]

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.

Comment 17

[Julien Cristau [:jcristau]]

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

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b45934914619

Comment 19

[[:philipp]]

i can confirm (on windows) that the 32bit & 64bit treeherder builds from https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&revision=b459349146198dbfee964c103644d25792a1a091&selectedJob=229677820 are no longer reproducibly crashing as opposed to 60.5.1esr on various reuters urls.

tested on:
https://www.reuters.com/article/us-iraq-usa-cash/u-s-sent-pallets-of-cash-to-baghdad-idUSN0631295120070207
https://www.reuters.com/article/us-usa-covington-suit/teen-in-lincoln-memorial-protest-sues-washington-post-for-250-million-idUSKCN1Q82SW
https://uk.reuters.com/article/uk-britain-oil-north-sea-volcanoes/phantom-volcanoes-may-hide-more-oil-and-gas-in-the-north-sea-study-idUKKCN1Q81KG

Comment 20

[Mike Taylor [:miketaylr]]

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?

Comment 21

[Adrian Florinescu [:aflorinescu]]

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.

Comment 22

[Mike Taylor [:miketaylr]]

(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.