accessing prototype of DOM objects throws uncatchable error

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: James Justin Harrell, Assigned: mrbkap)

Tracking

({regression, verified1.8.1.12})

unspecified
x86
Windows XP
regression, verified1.8.1.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.12 +
wanted1.8.1.x +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: regression from 316990, requires 392944)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

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.
examples:
for( let [propName, propValue] in DOMType.prototype ) {}
var obj = DOMType.prototype[propName];
DOMType.prototype.hasOwnProperty(propName);

Actual Results:  
NS_ERROR_XPC_BAD_OP_ON_WN_PROTO is thrown.

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.
(Reporter)

Comment 1

11 years ago
Created attachment 259647 [details]
tries to access HTMLElement.prototype.nodeName

For me, this always throws an exception and can always be caught.
(Reporter)

Comment 2

11 years ago
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

10 years ago
Can anyone confirm if this is a bug or not?

Comment 4

10 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
(Assignee)

Updated

10 years ago
Assignee: nobody → general
Component: XPConnect → JavaScript Engine
QA Contact: xpconnect → general
(Assignee)

Updated

10 years ago
Blocks: 316990
(Assignee)

Comment 5

10 years ago
Created attachment 277123 [details] [diff] [review]
Fix

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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #277123 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #277123 - Attachment description: FIx → Fix

Comment 6

10 years ago
So do we need this on branches too?

Can we add a test added for this?
Flags: in-testsuite?
Comment on attachment 277123 [details] [diff] [review]
Fix

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

/be
Attachment #277123 - Flags: review?(brendan) → review+
(Assignee)

Comment 8

10 years ago
Created attachment 277192 [details] [diff] [review]
Updated
Attachment #277123 - Attachment is obsolete: true
Attachment #277192 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Flags: blocking1.9+ → blocking1.9?
Attachment #277192 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

10 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: blocking1.8.1.7?
Depends on: 392944
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

10 years ago
It's a regression from bug 316990, which we took on branch.  Looks like we forgot to add the keyword when adding the dep.
Keywords: regression
Flags: blocking1.8.1.7? → blocking1.8.1.7+
blake: is this going to make 1.8.1.8? We need the fix for bug 392944 also if so, right?
Whiteboard: regression from 316990, requires 392944
I'll take blake's silence as a "no".
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Blake: are you available to fix this bug for 1.8.1.12 ? Please create a branch patch (that includes regression fix bug 392944) and request branch approval. Thx.
Flags: wanted1.8.1.x+
(Assignee)

Comment 15

10 years ago
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.
Attachment #294505 - Flags: review+
Attachment #294505 - Flags: approval1.8.1.12?
Comment on attachment 294505 [details] [diff] [review]
Rolled up patch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #294505 - Flags: approval1.8.1.12? → approval1.8.1.12+
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.

/be
(Assignee)

Comment 18

10 years ago
Already done, see comment 15, last sentence.
(Assignee)

Comment 19

10 years ago
Fixed on the 1.8 branch.
Keywords: fixed1.8.1.12
When I look at this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.12) Gecko/2008020121 Firefox/2.0.0.12, for the test case in comment 1, I see the same thing in 2.0.0.11 as 2.0.0.12. For the testcase in comment 2, 2.0.0.12 never shows the error in the error console while 2.0.0.11 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.
(Assignee)

Comment 21

10 years ago
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.
Thanks, Blake. I'm marking this then. I appreciate the information.
Keywords: fixed1.8.1.12 → verified1.8.1.12
verified fixed linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375344.js,v  <--  regress-375344.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-

Updated

8 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.