Crash [@ nsIDOMElement_GetTagName]

VERIFIED FIXED in Firefox 7

Status

()

Core
DOM
P1
critical
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla8
x86_64
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox7- fixed)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 539979 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

6 years ago
Created attachment 539980 [details]
stack trace
Is this a debug-only crash?  I can only reproduce this in a debug build.

In a debug build, this is failing the "QIs to nsIDOMElement" sanity check because the QI result is null.  The problem is that the unwrap is done to nsINode, which can work for non-element nodes...

This is fallout from bug 659539.

The right fix is to probably to add Element to the list of castable interfaces and use it here.
Blocks: 659539
Sounds like a good fix, but I'd like to hear peterv's opinion.
Assignee: nobody → jonas
tracking-firefox7: --- → ?
Except for two things:

1)  The castable interface macros don't work too hot with namespaced stuff.  We
    could use nsGenericElement to work around this.
2)  Things would _still_ fail for document fragments, since those cast to Element
    and nsGenericElement.  I suppose we could add an explicit check for that in
    the custom quickstub...

Jonas, thoughts?
And on a further note, I'm getting really tired of document fragment's inheritance stuff, and also tired of not having our new DOM binding unwrapping hotness... ;)
Well, if we're checking anyway we might as well leave things the way they are and check that the nodetype is ELEMENT_NODE.

Fixing the inheritance sounds good, would love to share a bit more between documents and elements. Hard to say if it's worth doing before we do the unwrapping goodness though?
> Well, if we're checking anyway we might as well leave things the way they are
> and check that the nodetype is ELEMENT_NODE.

Yeah, that works.

I wouldn't worry about the inheritance stuff for now.
We already have special casting macros for GenericElement, see http://hg.mozilla.org/mozilla-central/annotate/981ccfb8c1ae/js/src/xpconnect/src/nsDOMQS.h#l89 nsIDOMElement_GetTagName should just use |'thisType': 'nsGenericElement'|.
Oh, great.  Sicking, make it so!

Comment 10

6 years ago
It's not clear to me why this is nominated for release driver tracking.
It's a crash (in debug builds) and minor correctness (in opt builds) regression from a patch that landed in the Gecko 7 cycle.  Release drivers presumably need to decide whether to ship the regression, back out the patch that caused it, or take a fix for this.
Created attachment 544275 [details] [diff] [review]
Make sure that we're dealing with an element when getting tagName.
Attachment #544275 - Flags: review?(jonas)
Assignee: jonas → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 544275 [details] [diff] [review]
Make sure that we're dealing with an element when getting tagName.

Review of attachment 544275 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this!
Attachment #544275 - Flags: review?(jonas) → review+
Whiteboard: [need review] → [need landing]
http://hg.mozilla.org/integration/mozilla-inbound/rev/c688b177aa30
Flags: in-testsuite+
Whiteboard: [need landing]
Target Milestone: --- → mozilla8
Comment on attachment 544275 [details] [diff] [review]
Make sure that we're dealing with an element when getting tagName.

I think we should take this for aurora 7.  It's a zero-risk change that can help fuzzing and fixes a (slight) web compat regression.
Attachment #544275 - Flags: approval-mozilla-aurora?

Updated

6 years ago
tracking-firefox7: ? → -

Comment 16

6 years ago
Comment on attachment 544275 [details] [diff] [review]
Make sure that we're dealing with an element when getting tagName.

Approved for releases/mozilla-aurora
Attachment #544275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/mozilla-central/rev/c688b177aa30
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/releases/mozilla-aurora/rev/dbcb235ae784
status-firefox7: --- → fixed
Crash Signature: [@ nsIDOMElement_GetTagName]
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110818 Firefox/9.0a1
Mozilla/5.0 (Windows NT 6.1; rv:7.0a2) Gecko/20110816 Firefox/7.0a2

Firefox doesn't crash.
Setting resolution to Verified Fixed.
Thanks.
Status: RESOLVED → VERIFIED
If i am wrong please feel free to rechange the status.
Thanks!
I believe this might have been crashing in a debug build only according to Comment 11, so I am not sure you could verify it in regular build.

> If i am wrong please feel free to rechange the status.
> Thanks!
Yes, the crash is debug-only.  There's a correctness issue that was visible in opt builds; the automated testcases checked in with this bug (which are part of the patch) test that as well as the crash issue...
You need to log in before you can comment on or make changes to this bug.