664916 - Crash [@ nsIDOMElement_GetTagName]

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds like a good fix, but I'd like to hear peterv's opinion.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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... ;)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Peter Van der Beken [:peterv]]

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'|.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, great.  Sicking, make it so!

Comment 10

[Asa Dotzler [:asa]]

It's not clear to me why this is nominated for release driver tracking.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that we're dealing with an element when getting tagName.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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!

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c688b177aa30

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[LegNeato]

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

Approved for releases/mozilla-aurora

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/c688b177aa30

Comment 18

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/dbcb235ae784

Comment 19

[Trif Andrei-Alin[:AlinT]]

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.

Comment 20

[Trif Andrei-Alin[:AlinT]]

If i am wrong please feel free to rechange the status.
Thanks!

Comment 21

[Marcia Knous [:marcia]]

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!

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...