Closed Bug 1051858 Opened 5 years ago Closed 5 years ago

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

Categories

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

34 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox32 --- unaffected
firefox33 + verified
firefox34 --- verified

People

(Reporter: lizzard, Assigned: dmajor)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Blocks: 1051863
BatBrowseBAApp.dll
JotzeyBAApp.dll
DealKeeperBAApp.dll
etc. in the stacks.
Blocks: 1038243
What else could we try to fix the crash caused by those malwares.
Prevent loading of *BAApp.dll libraries ?
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?
Flags: needinfo?(lhenry)
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.
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.
Flags: needinfo?(lhenry)
Yes, whatever *BAApp.dll they personally have.
:tracy pointed out this is showing up on Aurora as [@ nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) ].  It is a top crash there, too.
Crash Signature: [@ nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)] → [@ nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)] [@ nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) ]
(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.
dmajor please rev the IIDs for Element/FragmentOrElement/nsIContent/nsINode/nsIDOMEventTarget
Assignee: nobody → dmajor
(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?
Gah, not sure how I missed that. nsIDOMElement and all subclasses, and nsIExpatSink need a uuid rev.
Attachment #8471964 - Flags: review?(bugs)
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.
Attachment #8471964 - Flags: review?(bugs) → review+
Flags: qe-verify+
Attached patch As landedSplinter Review
Not entirely sure we need those other IIDs, but OK I added them.
Attachment #8471964 - Attachment is obsolete: true
Attachment #8472693 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/494ab80e54f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Preliminary results from yesterday's two nightlies are looking good. No crashes after build 20140813030201.
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**
Attachment #8472693 - Flags: approval-mozilla-beta?
Attachment #8472693 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(jorge)
Flags: needinfo?(eshepherd)
[Tracking Requested - why for this release]: This is nearly half of our beta crashes
(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.
(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 on attachment 8472693 [details] [diff] [review]
As landed

I am taking it in aurora. Lawrence will decide for beta.
Attachment #8472693 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(Ms2ger)
(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.
Flags: needinfo?(Ms2ger)
Sounds like all we have are bad options. :(

(In reply to Robert Kaiser (:kairo@mozilla.com, 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?
Flags: needinfo?(kairo)
(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.
Flags: needinfo?(kairo)
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-
Attachment #8472693 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: needinfo?(jorge)
Flags: needinfo?(eshepherd)
Clearing tracking and marking 32 as unaffected because of the backout in bug 1038243.
(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.
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...
No new crash reports with these signatures for 34.0a1 or other versions after 2014-08-13 builds.

Marking as verified.
Status: RESOLVED → VERIFIED
QA Contact: petruta.rasa
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.