Last Comment Bug 375344 - accessing prototype of DOM objects throws uncatchable error
: accessing prototype of DOM objects throws uncatchable error
regression from 316990, requires 392944
: regression, verified1.8.1.12
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows XP
-- major with 1 vote (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
Depends on: 392944
Blocks: 316990
  Show dependency treegraph
Reported: 2007-03-25 18:12 PDT by James Justin Harrell
Modified: 2010-02-11 14:54 PST (History)
9 users (show)
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

tries to access HTMLElement.prototype.nodeName (102 bytes, text/html)
2007-03-25 18:19 PDT, James Justin Harrell
no flags Details
adds property then tries to access it (482 bytes, text/html)
2007-03-25 18:21 PDT, James Justin Harrell
no flags Details
Fix (975 bytes, patch)
2007-08-17 12:17 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
Updated (1.02 KB, patch)
2007-08-17 18:20 PDT, Blake Kaplan (:mrbkap)
jonas: approval1.9+
Details | Diff | Splinter Review
Rolled up patch (1.85 KB, patch)
2007-12-24 16:20 PST, Blake Kaplan (:mrbkap)
mrbkap: review+
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review

Description User image James Justin Harrell 2007-03-25 18:12:20 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070309 Firefox/
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070309 Firefox/

When trying to access a property of the prototype object of a DOM type, NS_ERROR_XPC_BAD_OP_ON_WN_PROTO is sometimes thrown, which cannot be caught. Sometimes DOMType.prototype.hasOwnProperty(propName) will throw an error while accessing DOMType.prototype[propName] won't, and vice-versa.

Bizarrely, sometimes the error isn't thrown immediately. Accessing the property goes fine but then shortly afterwards the error is thrown. The bug can go away and reemerge from seemingly unrelated changes.

Reproducible: Sometimes

Steps to Reproduce:
Try to access a property of the prototype object of a DOM type.
for( let [propName, propValue] in DOMType.prototype ) {}
var obj = DOMType.prototype[propName];

Actual Results:  

Expected Results:  
Nothing is thrown. The property evaluates to undefined or null if it doesn't make sense to access it from the prototype object. If the property does make sense, give the requested value.

I've also tested on a development build of Firefox 3 with a clean profile, and the bug still occurred.
Comment 1 User image James Justin Harrell 2007-03-25 18:19:27 PDT
Created attachment 259647 [details]
tries to access HTMLElement.prototype.nodeName

For me, this always throws an exception and can always be caught.
Comment 2 User image James Justin Harrell 2007-03-25 18:21:53 PDT
Created attachment 259648 [details]
adds property then tries to access it

For me, this always throws an exception when refreshed and the exception can never be caught.
Comment 3 User image Chris Shoemaker 2007-08-15 18:51:12 PDT
Can anyone confirm if this is a bug or not?
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-15 21:23:55 PDT
Hmm.   We're reporting the error with a delay.  Dunno why it's uncatchable, but not reporting it immediately is definitely a bug...  There's a JS_ReportPendingException missing somewhere, clearly.
Comment 5 User image Blake Kaplan (:mrbkap) 2007-08-17 12:17:05 PDT
Created attachment 277123 [details] [diff] [review]

There are two parts to this fix:
*) Note that the OBJ_GET_CLASS is now (correctly) on obj2 instead of obj. This was a bad typo that was causing us to throw the exception at all, obj2 doesn't have an outerObject hook.

*) We will now correctly propagate outerObject failure.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-17 12:25:50 PDT
So do we need this on branches too?

Can we add a test added for this?
Comment 7 User image Brendan Eich [:brendan] 2007-08-17 12:29:41 PDT
Comment on attachment 277123 [details] [diff] [review]

>+        JSObject *outer;
>+        clasp = OBJ_GET_CLASS(cx, obj2);
>         xclasp = (clasp->flags & JSCLASS_IS_EXTENDED)
>                  ? (JSExtendedClass *)clasp
>                  : NULL;
>+        if (!xclasp || !xclasp->outerObject) {

For some reason or other (maybe avoiding the lines, or the ternary with null else followed by null test that re-tests the ternary's condition), I prefer:

        clasp = OBJ_GET_CLASS(cx, obj2);
        if (!(clasp->flags & JSCLASS_IS_EXTENDED) ||
            !(xclasp = (JSExtendedClass *) clasp)->outerObject) {
            outer = NULL;
        } else {

r=me anyway you like.

Comment 8 User image Blake Kaplan (:mrbkap) 2007-08-17 18:20:52 PDT
Created attachment 277192 [details] [diff] [review]
Comment 9 User image Blake Kaplan (:mrbkap) 2007-08-17 18:25:02 PDT
Fix checked in.
Comment 10 User image Daniel Veditz [:dveditz] 2007-08-28 14:26:52 PDT
Why is this nominated for the branch? Doesn't appear to be a required stability fix or regression, and no one has yet alleged any security concerns.
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-28 15:58:32 PDT
It's a regression from bug 316990, which we took on branch.  Looks like we forgot to add the keyword when adding the dep.
Comment 12 User image Daniel Veditz [:dveditz] 2007-09-26 13:41:31 PDT
blake: is this going to make We need the fix for bug 392944 also if so, right?
Comment 13 User image Daniel Veditz [:dveditz] 2007-10-02 16:00:36 PDT
I'll take blake's silence as a "no".
Comment 14 User image Daniel Veditz [:dveditz] 2007-12-17 11:44:51 PST
Blake: are you available to fix this bug for ? Please create a branch patch (that includes regression fix bug 392944) and request branch approval. Thx.
Comment 15 User image Blake Kaplan (:mrbkap) 2007-12-24 16:20:31 PST
Created attachment 294505 [details] [diff] [review]
Rolled up patch

There was a slight conflict due to s/rval/vp/ renaming, but the fix was trivial, so I'm marking this r+. This patch includes the patch for bug 392944.
Comment 16 User image Daniel Veditz [:dveditz] 2008-01-07 14:49:48 PST
Comment on attachment 294505 [details] [diff] [review]
Rolled up patch

approved for, a=dveditz for release-drivers
Comment 17 User image Brendan Eich [:brendan] 2008-01-11 17:17:00 PST
mrbkap, can you please make the one-character(!) fix from the patch in bug 392944 before you commit, so I don't have to? Thanks.

Comment 18 User image Blake Kaplan (:mrbkap) 2008-01-12 10:04:40 PST
Already done, see comment 15, last sentence.
Comment 19 User image Blake Kaplan (:mrbkap) 2008-01-13 10:34:36 PST
Fixed on the 1.8 branch.
Comment 20 User image Al Billings [:abillings] 2008-02-05 13:43:13 PST
When I look at this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv: Gecko/2008020121 Firefox/, for the test case in comment 1, I see the same thing in as For the testcase in comment 2, never shows the error in the error console while does.

Is the test case in comment 1 not good? By the behavior in the other test case (from comment 2), I would think that this is fixed but I wanted to double check.
Comment 21 User image Blake Kaplan (:mrbkap) 2008-02-05 20:46:09 PST
Al, from what I can see, the testcase in comment 1 was a "control" testcase to show the exception being thrown and caught correctly. The testcase in comment 2 should no longer throw an exception thanks to the patch in this bug (the fact that we were throwing an exception at all was a bug).

In conclusion, this is (as you guessed) fixed.
Comment 22 User image Al Billings [:abillings] 2008-02-06 16:22:38 PST
Thanks, Blake. I'm marking this then. I appreciate the information.
Comment 23 User image Bob Clary [:bc:] 2008-03-25 04:53:27 PDT
verified fixed linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375344.js,v  <--  regress-375344.js
initial revision: 1.1

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