1051858 - crash in nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)

Status: VERIFIED FIXED

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

Description

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This bug was filed from the Socorro interface and is 
report bp-0fb8f2cc-cb72-48a5-aab8-c79862140810.
=============================================================

#5 topcrasher for 34.0a1 with 228/10099 crashes in the last 7 days. First appeared 2014-08-07 with the 20140807030202 build. 

regression range: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6cbdd4d523a7&tochange=afcb3af79d09

More reports: https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=7&range_unit=days&date=2014-08-11&signature=nsCOMPtr%3CnsIAttribute%3E%3A%3AnsCOMPtr%3CnsIAttribute%3E%28nsQueryInterface%29+|+mozilla%3A%3Adom%3A%3AAttr%3A%3AFromDOMAttr%28nsIDOMAttr*%29&version=Firefox%3A34.0a1#tab-reports

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

BatBrowseBAApp.dll
JotzeyBAApp.dll
DealKeeperBAApp.dll
etc. in the stacks.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

What else could we try to fix the crash caused by those malwares.
Prevent loading of *BAApp.dll libraries ?

Comment 3

[Benjamin Smedberg]

It would be nice to get our hands on the actual DLLs. Liz can you see if there are useful email addresses and contact people to see if they can send us the DLLs/about:support data/etc?

Comment 4

[Benjamin Smedberg]

Are we fairly confident that this is just some binary-incompatibility with a binary XPCOM calling nsGenericHTMLElement::SetAttributeNode when it intended to call some other DOM method?

I looked but didn't find any recent changes to Element/FragmentOrElement/nsIContent/nsINode/nsIDOMEventTarget would have changed the vtable layout of Element without changing the IID.

It's possible though revving some or all of those IIDs might fix this crash.

Comment 5

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

bsmedberg, sure, I can give that a try. Should I ask them only for the DLLs affected in their crash? They seem to be different DLLs for different reports.

Comment 6

[Benjamin Smedberg]

Yes, whatever *BAApp.dll they personally have.

Comment 7

[Andrew McCreight [:mccr8]]

:tracy pointed out this is showing up on Aurora as [@ nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) ].  It is a top crash there, too.

Comment 8

[Robert Kaiser]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> It's possible though revving some or all of those IIDs might fix this crash.

We definitely should investigate this for nightly, IMHO.

Comment 9

[Benjamin Smedberg]

dmajor please rev the IIDs for Element/FragmentOrElement/nsIContent/nsINode/nsIDOMEventTarget

Comment 10

[(Away)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Are we fairly confident that this is just some binary-incompatibility with a
> binary XPCOM calling nsGenericHTMLElement::SetAttributeNode when it intended
> to call some other DOM method?

I think so. The memory at the first parameter looks like an nsString: a pointer to the 0x00650068 text, followed by two small integers.

I had a nightly regression range in bug 1038243 and http://hg.mozilla.org/mozilla-central/rev/e6f113c83095 was my main suspect. That change did modify some vtables.

Does this mean that all the other interfaces in that change (nsIDOMElement, nsIDOMSVGElement, etc.) need to be revved as well?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Gah, not sure how I missed that. nsIDOMElement and all subclasses, and nsIExpatSink need a uuid rev.

Comment 12

[(Away)]

Attachment: ./mach update-uuids nsIDOMElement nsIExpatSink

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8471964 [details] [diff] [review]
./mach update-uuids nsIDOMElement nsIExpatSink

bsmedberg wanted to update also IIDs of Element/FragmentOrElement/nsIContent/nsINode/nsIDOMEventTarget.
rs+ for such change.

Comment 14

[(Away)]

Attachment: As landed

Not entirely sure we need those other IIDs, but OK I added them.

Comment 15

[(Away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/494ab80e54f8

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/494ab80e54f8

Comment 17

[(Away)]

Preliminary results from yesterday's two nightlies are looking good. No crashes after build 20140813030201.

Comment 18

[(Away)]

Comment on attachment 8472693 [details] [diff] [review]
As landed

Approval Request Comment
[Feature/regressing bug #]: bug 741295
[User impact if declined]: #1 crash on 32b (46%)
[Describe test coverage new/current, TBPL]: One night on m-c
[Risks and why]: This change revs the UUIDs on the interfaces that were changed by 741295. Addons will need to be updated to use the new IDs. Alternative is to back out 741295 but that seems even more risky.
[String/UUID change made/needed]: **Yes**

Comment 19

[(Away)]

We need to change some UUIDs on beta to fix a topcrash. Do we need to get this documented and/or talk to addon authors?

Comment 20

[(Away)]

[Tracking Requested - why for this release]: This is nearly half of our beta crashes

Comment 21

[Robert Kaiser]

(In reply to David Major [:dmajor] from comment #19)
> We need to change some UUIDs on beta to fix a topcrash.

AFAIK we cannot change UUIDs on beta as it's our "contract" with add-on devs that we do not do that (and it usually causes tons of crashes if we do). I'm not even 100% sure what the story on aurora is there.

Comment 22

[Robert Kaiser]

(In reply to David Major [:dmajor] from comment #20)
> [Tracking Requested - why for this release]: This is nearly half of our beta
> crashes

Note that this statement is not true for the latest beta (32.0b6), as we backed out the patch that aggravated the crashing. I don't know right now what the amount of crashes is right now, as it's hard to know which 3rd-party signatures exactly are to be counted into this.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8472693 [details] [diff] [review]
As landed

I am taking it in aurora. Lawrence will decide for beta.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38a71dd3290c

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

1. Sylvestre - We missed the need to rev the UUID in our check. We should add this check to the tool.
2. Jorge - See comment 19, how can we measure the impact on add-ons if we choose to rev the UUIDs at this late stage of beta?
3. ms2ger - Can you comment on the feasibility of backing out the sizable patch in bug 741295 and shipping that change in 33 instead of 32?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Lawrence Mandel [:lmandel] from comment #25)
> 3. ms2ger - Can you comment on the feasibility of backing out the sizable
> patch in bug 741295 and shipping that change in 33 instead of 32?

Somewhat risky, and breaks our contract with addon authors even worse than the uuid bump, because it changes binary compat.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Sounds like all we have are bad options. :(

(In reply to Robert Kaiser (:[e-mail], slow reaction due to vacation backlog) from comment #22)
> (In reply to David Major [:dmajor] from comment #20)
> > [Tracking Requested - why for this release]: This is nearly half of our beta
> > crashes
> 
> Note that this statement is not true for the latest beta (32.0b6), as we
> backed out the patch that aggravated the crashing. I don't know right now
> what the amount of crashes is right now, as it's hard to know which
> 3rd-party signatures exactly are to be counted into this.

Kairo - I would like to determine whether we should live with the crash in 32. Can you figure out how bad this crash is in beta6 and beta7?

Comment 28

[Robert Kaiser]

(In reply to Lawrence Mandel [:lmandel] from comment #27)
> Kairo - I would like to determine whether we should live with the crash in
> 32. Can you figure out how bad this crash is in beta6 and beta7?

As I mentioned, this crash here is gone from b6 and b7, i.e. after the backout in bug 1038243, so not an issue any more. That said, the original signature from that bug is also not present in the top 300 for any of those versions. Given that, I would not do anythi9ng to 32 at this point.

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8472693 [details] [diff] [review]
As landed

Based on the information in comment 28 that this crash is no longer present on beta 32, the best option is to do nothing. beta-

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Clearing tracking and marking 32 as unaffected because of the backout in bug 1038243.

Comment 31

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to David Major [:dmajor] from comment #19)
> We need to change some UUIDs on beta to fix a topcrash. Do we need to get
> this documented and/or talk to addon authors?

Responding for the record: if UUID or other add-on compat-breaking changes are needed on beta, we document them on the blog or notify add-on devs directly if necessary. Of course, it's always better to find alternate solutions.

Comment 32

[(Away)]

I am at a loss to explain why the previous signature hasn't sprung back up after the backout of 1038243. I see a handful of results on 32b7 but in extremely low volume. There hasn't been a change to the external binary; I don't see any newer versions in other reports. I doubt that we simply lost all the affected users since it wasn't a startup crash.

Without a good explanation, I worry that this hasn't been fully solved. I guess we'll have to wait and see...

Comment 33

[Petruta Horea [:phorea]]

No new crash reports with these signatures for 34.0a1 or other versions after 2014-08-13 builds.

Marking as verified.