Last Comment Bug 664916 - Crash [@ nsIDOMElement_GetTagName]
: Crash [@ nsIDOMElement_GetTagName]
Status: VERIFIED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Mac OS X
: P1 critical (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 326633 659539
  Show dependency treegraph
 
Reported: 2011-06-16 20:15 PDT by Jesse Ruderman
Modified: 2013-12-27 14:20 PST (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
testcase (crashes Firefox when loaded) (247 bytes, text/html)
2011-06-16 20:15 PDT, Jesse Ruderman
no flags Details
stack trace (8.02 KB, text/plain)
2011-06-16 20:17 PDT, Jesse Ruderman
no flags Details
Make sure that we're dealing with an element when getting tagName. (3.12 KB, patch)
2011-07-06 09:41 PDT, Boris Zbarsky [:bz]
jonas: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-06-16 20:15:49 PDT
Created attachment 539979 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2011-06-16 20:17:42 PDT
Created attachment 539980 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] 2011-06-16 20:28:46 PDT
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 2011-06-16 20:31:58 PDT
Sounds like a good fix, but I'd like to hear peterv's opinion.
Comment 4 Boris Zbarsky [:bz] 2011-06-16 20:34:57 PDT
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 [:bz] 2011-06-16 20:35:47 PDT
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 2011-06-16 21:11:43 PDT
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 [:bz] 2011-06-16 21:33:23 PDT
> 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] 2011-06-29 13:52:46 PDT
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 [:bz] 2011-06-29 14:02:20 PDT
Oh, great.  Sicking, make it so!
Comment 10 Asa Dotzler [:asa] 2011-07-05 19:57:03 PDT
It's not clear to me why this is nominated for release driver tracking.
Comment 11 Boris Zbarsky [:bz] 2011-07-05 21:09:06 PDT
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 [:bz] 2011-07-06 09:41:47 PDT
Created attachment 544275 [details] [diff] [review]
Make sure that we're dealing with an element when getting tagName.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-06 11:30:40 PDT
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 15 Boris Zbarsky [:bz] 2011-07-07 12:46:46 PDT
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 christian 2011-07-07 14:36:42 PDT
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] 2011-07-08 05:52:53 PDT
http://hg.mozilla.org/mozilla-central/rev/c688b177aa30
Comment 19 Trif Andrei-Alin[:AlinT] 2011-08-19 07:42:38 PDT
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] 2011-08-19 07:43:19 PDT
If i am wrong please feel free to rechange the status.
Thanks!
Comment 21 Marcia Knous [:marcia - use ni] 2011-08-19 16:28:40 PDT
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 [:bz] 2011-08-19 19:53:50 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.