Closed
Bug 1051858
Opened 9 years ago
Closed 9 years ago
crash in nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla34
Tracking | Status | |
---|---|---|
firefox32 | --- | unaffected |
firefox33 | + | verified |
firefox34 | --- | verified |
People
(Reporter: lizzard, Assigned: away)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(1 file, 1 obsolete file)
13.68 KB,
patch
|
away
:
review+
Sylvestre
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
BatBrowseBAApp.dll JotzeyBAApp.dll DealKeeperBAApp.dll etc. in the stacks.
Blocks: 1038243
Comment 2•9 years ago
|
||
What else could we try to fix the crash caused by those malwares. Prevent loading of *BAApp.dll libraries ?
Comment 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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.
Reporter | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
Yes, whatever *BAApp.dll they personally have.
Comment 7•9 years ago
|
||
: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**) ]
Updated•9 years ago
|
status-firefox32:
--- → affected
status-firefox33:
--- → affected
![]() |
||
Comment 8•9 years ago
|
||
(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•9 years ago
|
||
dmajor please rev the IIDs for Element/FragmentOrElement/nsIContent/nsINode/nsIDOMEventTarget
Assignee: nobody → dmajor
![]() |
Assignee | |
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
Gah, not sure how I missed that. nsIDOMElement and all subclasses, and nsIExpatSink need a uuid rev.
![]() |
Assignee | |
Comment 12•9 years ago
|
||
Attachment #8471964 -
Flags: review?(bugs)
Comment 13•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Flags: qe-verify+
![]() |
Assignee | |
Comment 14•9 years ago
|
||
Not entirely sure we need those other IIDs, but OK I added them.
Attachment #8471964 -
Attachment is obsolete: true
Attachment #8472693 -
Flags: review+
![]() |
Assignee | |
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/494ab80e54f8
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/494ab80e54f8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
![]() |
Assignee | |
Comment 17•9 years ago
|
||
Preliminary results from yesterday's two nightlies are looking good. No crashes after build 20140813030201.
![]() |
Assignee | |
Comment 18•9 years ago
|
||
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?
![]() |
Assignee | |
Comment 19•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 20•9 years ago
|
||
[Tracking Requested - why for this release]: This is nearly half of our beta crashes
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
![]() |
||
Comment 21•9 years ago
|
||
(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•9 years ago
|
||
(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.
Updated•9 years ago
|
Comment 23•9 years ago
|
||
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+
Comment 25•9 years ago
|
||
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)
Comment 26•9 years ago
|
||
(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)
Comment 27•9 years ago
|
||
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)
![]() |
||
Comment 28•9 years ago
|
||
(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 29•9 years ago
|
||
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)
Comment 30•9 years ago
|
||
Clearing tracking and marking 32 as unaffected because of the backout in bug 1038243.
tracking-firefox32:
+ → ---
Comment 31•9 years ago
|
||
(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.
![]() |
Assignee | |
Comment 32•9 years ago
|
||
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•9 years ago
|
||
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
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•