1313340 - Crashes with mozilla::ErrorResult::ClearDOMExceptionInfo since Firefox 49

Status: RESOLVED DUPLICATE of bug 1328861

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-c1965f44-7274-408c-b952-b6fee2161027.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	je_free 	memory/mozjemalloc/jemalloc.c:6479
1 	xul.dll 	mozilla::ErrorResult::ClearDOMExceptionInfo() 	dom/bindings/BindingUtils.cpp:351
2 	xul.dll 	mozilla::ErrorResult::ClearUnionData() 	dom/bindings/BindingUtils.cpp:372
3 	xul.dll 	nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) 	xpcom/glue/nsCOMPtr.cpp:125
4 	xul.dll 	mozilla::dom::XPathEvaluator::CreateExpression(nsAString_internal const&, mozilla::dom::XPathNSResolver*, mozilla::ErrorResult&) 	dom/xslt/xpath/XPathEvaluator.cpp:126
5 	xul.dll 	mozilla::dom::XPathEvaluator::Evaluate(JSContext*, nsAString_internal const&, nsINode&, mozilla::dom::XPathNSResolver*, unsigned short, JS::Handle<JSObject*>, mozilla::ErrorResult&) 	dom/xslt/xpath/XPathEvaluator.cpp:187
6 	xul.dll 	mozilla::dom::DocumentBinding::evaluate 	obj-firefox/dom/bindings/DocumentBinding.cpp:10671
7 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:440
8 	xul.dll 	InternalCall 	js/src/vm/Interpreter.cpp:497
9 	xul.dll 	nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/generic/nsBlockFrame.cpp:1454
10 	xul.dll 	nsPerformanceStatsService::StopwatchStart(unsigned __int64) 	toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1130

cross-platform crashes with mozilla::ErrorResult::ClearDOMExceptionInfo in the signature seem to be regressing since firefox 49 builds.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Any thoughts on this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The obvious thought is that the stack in that incident report doesn't make much sense.

The line cited in frame 4 is this line in CreateExpression:

    return CreateExpression(aExpression, &pContext, doc, aRv);

and the function being called is http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/dom/xslt/xpath/XPathEvaluator.cpp#140

That function _does_ interact with aRv in the following ways:

1)  Assigns an nsresult to it (the return value of txExprParser::createExpr().
2)  Calls SuppressException() on it in some cases.
3)  Calls Throw() on it if it called SuppressException.

none of those go through nsCOMPtr_base::assign_from_helper.  The operator=(nsresult) can't call ClearUnionData, ever, nor can the Throw() call.  SuppressException _does_ call ClearUnionData, so maybe that's really the codepath being followed?  But to call ClearDOMExceptionInfo under ClearUnionData we would need to have mResult == NS_ERROR_DOM_DOMEXCEPTION, which is only set by ErrorResult::ThrowDOMException.  And this codepath never calls ThrowDOMException.

Oh, and frames 9 and 10 of the stack make no sense either, but maybe that's because jitcode confused the stackwalker?

OK, what if if we assume the stack is just slightly wonky instead of completely garbage... 

The numeric value of NS_ERROR_DOM_DOMEXCEPTION is (1<<31)|((14 + 0x45) << 16) | 1017) = 0x805303f9.

The possible error return values of txExprParser::createExpr are:

NS_ERROR_XPATH_INVALID_VAR_NAME == 0x80600013
NS_ERROR_XPATH_UNEXPECTED_END == 0x80600014
NS_ERROR_XPATH_OPERATOR_EXPECTED == 0x80600015
NS_ERROR_XPATH_UNCLOSED_LITERAL == 0x80600016
NS_ERROR_XPATH_BAD_COLON == 0x80600017
NS_ERROR_XPATH_BAD_BANG == 0x80600018
NS_ERROR_XPATH_ILLEGAL_CHAR == 0x80600019
NS_ERROR_XPATH_BINARY_EXPECTED == 0x8060001a

None of those looks terribly close to NS_ERROR_DOM_DOMEXCEPTION...

Getting something like NS_ERROR_DOM_DOMEXCEPTION via a bitflip is also not easy.  The only other error with "1017" as the module-specific error code is NS_ERROR_HTMLPARSER_HIERARCHYTOODEEP, but NS_ERROR_MODULE_HTMLPARSER + 0x45 is 0x4e, whereas NS_ERROR_MODULE_DOM + 0x45 is 0x53, which have 4 bits of difference between them.  And within NS_ERROR_MODULE_DOM, the only things that are a single bit away from NS_ERROR_DOM_DOMEXCEPTION are:

NS_ERROR_DOM_SECMAN_ERR
NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN
NS_ERROR_DOM_MALFORMED_URI
NS_ERROR_DOM_INVALID_STATE_XHR_MUST_BE_OPENED
NS_ERROR_UNCATCHABLE_EXCEPTION

as far as I can see.  None would really arise in this xslt code.  So I can't even easily blame this on a bitflip in mResult.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What confuses me a bit is that we have a _lot_ (well, dozens at least) of crashes with ClearDOMExceptionInfo in the signature, all of them (or at least the 20 or so I looked at) coming through about this codepath according to crash-stats.

I tried looking for other ClearUnionData() crashes, but can't figure out how to search crash-stats for "something in the stack" as opposed to "signature".  

I guess my list of "possible return values" above isn't quite right, because I missed the other createExpr signature, which can fail with things like NS_ERROR_OUT_OF_MEMORY and others (e.g. NS_ERROR_UNEXPECTED).  So maybe we are in fact getting single bit flips from one of the other one-bit-away DOM error codes?

Comment 4

[Andrew Overholt [:overholt]]

(P3 because low-ish volume but analysis is going on here so happy to change)

Comment 5

[[:philipp]]

> I tried looking for other ClearUnionData() crashes, but can't figure out how
> to search crash-stats for "something in the stack" as opposed to
> "signature".  

you can do that with the "proto signature" field which includes the stack of the crashing thread.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> you can do that with the "proto signature" field

Aha!

So doing that shows me a bunch of crashes that go through ClearUnionData to ClearDOMExceptionInfo to ReleaseData() and then crash.  And they're all also claimed to be coming from XPathEvaluator::CreateExpression which is being called via DocumentBinding::evaluate, just like the stack in this bug.

Those reports are all being labeled with bug 1145613 in crash-stats, but that's a fairly useless catch-all that just means "operating on a string in garbage memory"...  The underlying cause for those (the ones passing through ClearUnionData()) is presumably the same as this bug.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Reports still coming in with 52 on Beta :(

Comment 9

[Chris Peterson [:cpeterson]]

bz thinks the STR from bug 1328861 comment 6 are relevant to this crash.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bug 1328861 is a dup of this one, but as of 11 hours ago has steps to reproduce and as of 30 minutes ago has this pinned to what I expect is the right regressing changeset.  Working on debugging this now.  ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> And this codepath never calls ThrowDOMException.

To be clear: I was wrong about it.  That codepath takes a webidl callback that _can_ call ThrowDOMException, and then it can propagate out just the nsresult value from it (not the whole ErrorResult).

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixed in 52 in the duplicate.