Closed
Bug 1204324
Opened 9 years ago
Closed 9 years ago
crash in nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<T> const&) rising in 41.0b9
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: philipp, Unassigned)
References
Details
(Keywords: crash)
Crash Data
This bug was filed from the Socorro interface and is
report bp-027c23e1-4696-44f3-a423-b52b72150913.
=============================================================
0 xul.dll nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) dom/base/nsXMLHttpRequest.cpp
1 xul.dll nsXMLHttpRequest::Send(nsXMLHttpRequest::RequestBody const&) dom/base/nsXMLHttpRequest.h
2 xul.dll nsXMLHttpRequest::Send(JSContext*, nsAString_internal const&, mozilla::ErrorResult&) dom/base/nsXMLHttpRequest.h
3 xul.dll mozilla::dom::XMLHttpRequestBinding::send obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp
4 xul.dll mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp
5 xul.dll js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp
6 xul.dll js::TypeScript::Monitor(JSContext*, JSScript*, unsigned char*, JS::Value const&) js/src/vm/TypeInference-inl.h
7 xul.dll Interpret js/src/vm/Interpreter.cpp
8 xul.dll js::frontend::BytecodeEmitter::bindNameToSlot(js::frontend::ParseNode*) js/src/frontend/BytecodeEmitter.cpp
9 xul.dll js::frontend::BytecodeEmitter::emitNameOp(js::frontend::ParseNode*, bool) js/src/frontend/BytecodeEmitter.cpp
10 xul.dll js::frontend::BytecodeEmitter::emitCallOrNew(js::frontend::ParseNode*) js/src/frontend/BytecodeEmitter.cpp
11 xul.dll js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*) js/src/frontend/BytecodeEmitter.cpp
12 xul.dll Evaluate js/src/jsapi.cpp
13 xul.dll nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp
14 xul.dll nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) dom/base/nsJSUtils.cpp
15 xul.dll nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&, void**) dom/base/nsScriptLoader.cpp
this crash is on the rise after the 41.0b9 release. at the moment it takes place #1 in the top score board with 7 crashes per installation and 71% during startup. when looking through the reports, this crash seems correlated to the presence of one of those extensions most of the time:
{d49175b3-3fd8-43b8-b28e-da5d47f3c398} 1.0.59 COMPUTERBILD-Abzockschutz
firefox-support@vworldc.com vb-20150728103734 VWC Cocoon
Comment 1•9 years ago
|
||
[Tracking Requested - why for this release]:
As philipp says, this is #1 in Top Crash Score as it happens to ~70% at startup with 3.6 crashes per installation, so really annoying to users.
tracking-firefox41:
--- → ?
uploadChannel is null, meaning that uploadChannel2 must have also been null, meaning that this would have triggered on a debug build:
// This assertion will fire if buggy extensions are installed
NS_ASSERTION(uploadChannel2, "http must support nsIUploadChannel2");
Could this be related to the disabled addon signing requirement in b9?
> correlated to the presence of one of those extensions most of the time:
> {d49175b3-3fd8-43b8-b28e-da5d47f3c398} 1.0.59 COMPUTERBILD-Abzockschutz
Our correlations files are broken again, but in a spot-check I do see this 1.0.59 extension frequently.
And other data generally agrees: I see 74% "de" locale, and some of the top URLs are computerbild.de and amazon.de.
Reporter | ||
Comment 4•9 years ago
|
||
the other mentioned addon that might be related to this crash is more prevalent in en-us locales
firefox-support@vworldc.com vb-20150728103734 VWC Cocoon
i could reproduce this crash with "COMPUTERBILD-Abzockschutz 1.0.59" on beta 41.0b9 but not with 40.0.3 - all that was necessary to trigger it was a visit amazon.de, which causes a crash loop after the browser is restarted as well.
the regression range is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8e1657e7e7433a561c6e84d3820dc16471647158&tochange=4819768c871f5f64c5e2a984c7794f5385bc2977
> the regression range is
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=8e1657e7e7433a561c6e84d3820dc16471647158&tochange=4819
> 768c871f5f64c5e2a984c7794f5385bc2977
Thanks Philipp!
I have a feeling that this is an ordinary case of "codebase changed and old addons can't deal with it" but I'd like to give Christoph a chance to weigh in before we block these addons. Do you see any indication that this bug is unusual?
Flags: needinfo?(mozilla)
Comment 6•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #5)
> I have a feeling that this is an ordinary case of "codebase changed and old
> addons can't deal with it" but I'd like to give Christoph a chance to weigh
> in before we block these addons. Do you see any indication that this bug is
> unusual?
If this line [1] would fire in a debug build, then I suppose it's fine to disable those addons. Jonas wrote that code, he is probably in a better position to answer your question. So I am deferring to him.
[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#2762
Flags: needinfo?(jonas)
Comment 7•9 years ago
|
||
Also clearing my needinfo request.
Updated•9 years ago
|
Flags: needinfo?(mozilla)
Jorge, there are at least two addons that may not be blocked before we gtb 41 RC in the next hour or so. Would you be able to update the addon blocklist and nominate a patch for uplift to Beta/m-r? Many thanks!
Flags: needinfo?(jorge)
*may need to be blocked I mean.
Reporter | ||
Comment 10•9 years ago
|
||
the crash is reproducible when installing the addon available from https://www.getcocoon.com/support/download (firefox-support@vworldc.com vb-20150728103734) on 41.0b9 as well: https://crash-stats.mozilla.com/report/index/ac0e999c-4157-4583-abcf-6453b2150914
Comment 11•9 years ago
|
||
Jorge - to summarize, it's these two:
{d49175b3-3fd8-43b8-b28e-da5d47f3c398} 1.0.59
firefox-support@vworldc.com vb-20150728103734
Philipp was able to reproduce crashes with each of those.
We should probably also reach out to that support address.
Comment 12•9 years ago
|
||
Okay, I marked both add-ons as incompatible with 41 and above.
Updated•9 years ago
|
Flags: needinfo?(jorge)
Tracking this bug for 41 RC week so we can keep an eye on any/all other addons that might similarly be impacting firefox stability.
status-firefox41:
--- → affected
Comment 14•9 years ago
|
||
After some discussion on IRC it's been decided that it's best to go with the blocklist than the compatibility override, since the startup crashes might only be properly prevented with the blocklist that the user has in the profile or built into Firefox.
There's very little information here to go on to figure out what's going wrong.
Actually, I think the problem here is that we need to make nsSecCheckWrapChannelBase implement nsIUploadChannel and nsIUploadChannel2.
Alternatively the addon could implement nsIProtocolHandler.newChannel2, but that also means that the addon needs to take on the responsibility to do various security checks, which is not easy to do currently.
Christoph, don't we also need to make nsSecCheckWrapChannelBase implement AsyncOpen2?
bhearsum, jlund: Given that the addons blocklist was updated, I was told that RelEng will need to an update on their infra to pick up this change before I gtb 41 RC. Thanks for your help guys!
Flags: needinfo?(jlund)
Flags: needinfo?(bhearsum)
Comment 17•9 years ago
|
||
The blocklist has been updated just now.
Comment 18•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #15)
> Christoph, don't we also need to make nsSecCheckWrapChannelBase implement
> AsyncOpen2?
Currently, we only perform call forwarding [1]. So if the underlying channel does not implement asyncOpen2(), then an error is returned. I think we should implement that wrapping functionality, similar to what we do for ::GetLoadInfo().
Jonas, I think we could get the loadInfo from the outer channel and call into the contentSecurityManager ourselves, right? Should we go ahead and do that?
[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSecCheckWrapChannel.h#48
Reporter | ||
Comment 19•9 years ago
|
||
geolocater (https://addons.mozilla.org/firefox/addon/geolocater/) exhibits the same crash as well:
https://crash-stats.mozilla.com/report/index/6e45a255-bb9b-48b3-aafd-67a362150914
Comment 20•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #16)
> bhearsum, jlund: Given that the addons blocklist was updated, I was told
> that RelEng will need to an update on their infra to pick up this change
> before I gtb 41 RC. Thanks for your help guys!
Jordan is taking care of this.
Flags: needinfo?(bhearsum)
Comment 22•9 years ago
|
||
(In reply to Ben Hearsum (:bhearsum) from comment #20)
> (In reply to Ritu Kothari (:ritu) from comment #16)
> > bhearsum, jlund: Given that the addons blocklist was updated, I was told
> > that RelEng will need to an update on their infra to pick up this change
> > before I gtb 41 RC. Thanks for your help guys!
>
> Jordan is taking care of this.
I had to trigger the job that updates blocklist twice as the first time it thought there was no changes. there may have been a caching race condition when the first one polled addons.m.o
this should be done now: https://hg.mozilla.org/releases/mozilla-release/rev/5b7b83db947a
Flags: needinfo?(jlund)
Updated•9 years ago
|
Flags: qe-verify+
QA Contact: vasilica.mihasca
Now that bug 1204648 has landed, can anyone check if these addons work better? If they are still crashing, then a stack would be most helpful.
Flags: needinfo?(jonas)
Comment 24•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #23)
> Now that bug 1204648 has landed, can anyone check if these addons work
> better? If they are still crashing, then a stack would be most helpful.
We won't see any effects this quickly. The nightly and aurora populations generally don't have these kinds of buggy addons. (And unsigned addons are still disabled on 43 anyway, no?)
Comment 10 made it sound like the crash had been reproduced locally. So I was hoping that we could re-test the same steps on a build after bug 1204648 had landed.
Though I'm seeing now that comment 10 wasn't referring to nightly builds, so maybe that would be a bunch of work still.
Reporter | ||
Comment 26•9 years ago
|
||
yes, the crashes are easily reproducible with the mentioned addons in nightly as well. i'll test it with tomorrow's nightly again and report back...
(adding note to self)
Flags: needinfo?(madperson)
Reporter | ||
Comment 27•9 years ago
|
||
i tried it with the tinderbox build by dmajor's recommendation and it no longer crashes with any of the three extensions mentioned before, now that bug 1204648 had landed.
Flags: needinfo?(madperson)
Potentially we could uplift bug 1204648 then. I think the patch is pretty safe.
Comment 29•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #28)
> Potentially we could uplift bug 1204648 then. I think the patch is pretty
> safe.
Yep, already did:
https://bugzilla.mozilla.org/show_bug.cgi?id=1204648#c9
Comment 30•9 years ago
|
||
I just realized, I don't understand why bug 1204648 fixes the bug, if that patch only applies to 42 and 43? Can it not be uplifted to 41?
Jonas, perhaps David's question in comment 30 is directed at you.
Flags: needinfo?(jonas)
It should apply fine to 41 as well, but Christoph should confirm as it is his patch.
Flags: needinfo?(jonas) → needinfo?(mozilla)
Comment 33•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #32)
> It should apply fine to 41 as well, but Christoph should confirm as it is
> his patch.
Well, in Bug 1143922 we added asyncOpen2 to nsIChannel, which landed in 42. Which means that we can *not* uplift to 41. The patch of Bug 1204648 shouldn't even compile in 41 and there shouldn't be any issue in 41. So at the moment I don't quite understand what the problem in 41 was, but it seems it's not related to the problem we fixed in Bug 1204648.
Flags: needinfo?(mozilla)
After uplifting the fix from Bug 1205410, I want to call this "fixed". Please let me know if there are any concerns.
Comment 35•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #34)
> After uplifting the fix from Bug 1205410, I want to call this "fixed".
> Please let me know if there are any concerns.
The fix in Bug 1205410 does not fix the problem described in this patch. The right patch for *41* to fix the problem described here would be this patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=1204648#c18
which was *not* selected for 41.
Flags: needinfo?(rkothari)
Comment 36•9 years ago
|
||
Umm, as far as I understood, this bug was filed for the crash in 41, and bug 1205410 fixes that crash in 41, doesn't it?
Comment 37•9 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #36)
> Umm, as far as I understood, this bug was filed for the crash in 41, and bug
> 1205410 fixes that crash in 41, doesn't it?
Sorry, my bad. You are absolutely right Robert. I was confused. Since we basically disabled the channel wrapper for 41, we are not facing the problem described in this bug for 41. Sorry for the confusion.
Flags: needinfo?(rkothari)
Reporter | ||
Comment 38•9 years ago
|
||
marking this bug as fixed then, also based on the available crash stats (it's basically gone since rc3 and in 41.0)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•