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) (please use needinfo!)
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) (please use needinfo!)
brendan: review+
Details | Diff | Review
Updated (1.02 KB, patch)
2007-08-17 18:20 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jonas: approval1.9+
Details | Diff | Review
Rolled up patch (1.85 KB, patch)
2007-12-24 16:20 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
mrbkap: review+
dveditz: approval1.8.1.12+
Details | Diff | Review

Description 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 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 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 Chris Shoemaker 2007-08-15 18:51:12 PDT
Can anyone confirm if this is a bug or not?
Comment 4 Boris Zbarsky [:bz] 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 Blake Kaplan (:mrbkap) (please use needinfo!) 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 Boris Zbarsky [:bz] 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 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 Blake Kaplan (:mrbkap) (please use needinfo!) 2007-08-17 18:20:52 PDT
Created attachment 277192 [details] [diff] [review]
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2007-08-17 18:25:02 PDT
Fix checked in.
Comment 10 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 Boris Zbarsky [:bz] 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 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 Daniel Veditz [:dveditz] 2007-10-02 16:00:36 PDT
I'll take blake's silence as a "no".
Comment 14 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 Blake Kaplan (:mrbkap) (please use needinfo!) 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 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 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 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-01-12 10:04:40 PST
Already done, see comment 15, last sentence.
Comment 19 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-01-13 10:34:36 PST
Fixed on the 1.8 branch.
Comment 20 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 Blake Kaplan (:mrbkap) (please use needinfo!) 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 Al Billings [:abillings] 2008-02-06 16:22:38 PST
Thanks, Blake. I'm marking this then. I appreciate the information.
Comment 23 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.