Closed Bug 393267 Opened 17 years ago Closed 16 years ago

js1_5/extensions/scope-001.js FAIL browser only

Categories

(Core :: JavaScript Engine, defect, P2)

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bc, Assigned: mrbkap)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

forked from bug 390948. test began failing 8/2-8/3. once bug 390948 was fixed, it showed this different error.

BUGNUMBER: 53268
STATUS: Testing scope after changing obj.__proto__
PASSED! Step 1: setting obj.__proto__ = global object
FAILED! expected: [reported from test()] Expected value '5', Actual value '1'
FAILED! expected: [reported from test()] Type mismatch, expected type undefined, actual type number Expected value 'undefined', Actual value '1'
PASSED! Step 2: setting obj.__proto__ = null
PASSED! Step 3: setting obj.__proto__ to global object again
PASSED! Step 3: setting obj.__proto__ to global object again
PASSED! Step 4: setting obj.__proto__ to null again
PASSED! Step 4: setting obj.__proto__ to null again
Flags: in-testsuite+
Flags: blocking1.9?
Attached patch Uncompiled patch (obsolete) — Splinter Review
I think this should fix the bug -- I'll compile and test in a bit.
Assignee: crowder → mrbkap
Status: NEW → ASSIGNED
Comment on attachment 281326 [details] [diff] [review]
Uncompiled patch

There's an updated version of this patch in bug 393269 now. Note that the patch actually causes *more* tests failures on this bug.
Attachment #281326 - Attachment is obsolete: true
Are this bug and bug 393269 dupable, then?  Does that patch still make this problem -worse-?
No, the problem here is that after setting obj.__proto__ = XOW, setting obj.__proto__ = null doesn't work. The problem is that we end up finding __proto__ on the XOW, which then sets its wrapped object's prototype (oops). I can come up with a hack-patch for this without too much trouble, though.
Oh, and the reason the patch in bug 393269 will cause more test failures here is that we're failing to propagate the const attributes to properties on the XOW allowing them to be overwritten. That appears to make the testcase for this bu better since it looks like we're adding new properties to obj instead of simply overwriting the existing ones.
Attached patch Ugly brutal fixSplinter Review
This is ugly but it gets the job done. The problem here is that given |obj.__proto__ = someXOW; obj.__proto__ = notXOW;| we resolve __proto__ the second time to mean the XOW's wrapped object's __proto__ and change that. The gist of this fix is to detect that we don't actually want to set the XOWs proto if the original object isn't the XOW. The problem is that we don't know if the XOW's prototype chain ends in Object.prototype, and there's no easy way to check if it does, so we allow the JS engine to set the XOW's prototype and then go back and fix up both prototype chains. Hopefully, this isn't too common, and I'm fine with penalizing whoever does it.
Attachment #287959 - Flags: superreview?(brendan)
Attachment #287959 - Flags: review?
Attachment #287959 - Flags: review? → review?(jst)
Comment on attachment 287959 [details] [diff] [review]
Ugly brutal fix

Fun.
Attachment #287959 - Flags: review?(jst) → review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment on attachment 287959 [details] [diff] [review]
Ugly brutal fix


>+    // If code is trying to set obj.__proto__ and we're on obj's
>+    // prototype chain, then the OBJ_SET_PROPERTY above will do the
>+    // wrong thing if wrappedObj still delegates to Object.prototype.
>+    // However, it's hard to figure out if wrappedObj still does
>+    // delegate to Object.prototype so check to see if proto changed as a
>+    // result of setting __proto__.
>+
>+    if (origObj != obj) {
>+      // Undo the damage.
>+      if (!JS_SetPrototype(cx, wrappedObj, proto) ||
>+          !JS_SetPrototype(cx, origObj, newProto)) {
>+        return JS_FALSE;
>+      }
>+    } else if (newProto) {
>+      // __proto__ setting is a bad hack, people shouldn't do it. In the
>+      // interests of sanity, only allow them to set XOW wrapped protos
>+      // to null.
>+
>+      JS_SetPrototype(cx, wrappedObj, proto);
>+      JS_ReportError(cx, "invalid __proto__ value (can only be set to null)");
>+      return JS_FALSE;
>+    }

Nit: comment in the else if (newProto) {...} consequent about how this case is where the wrapper is direct, not on the prototype chain (if I'm following this at all)?

/be
Attachment #287959 - Flags: superreview?(brendan) → superreview+
do we want to check this in and close it?
mrbkap, can we get this in ?
I just landed this, with an updated comment. Blake, please have a look at the new comment and verify that it still reflects reality :)
Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
js1_5/extensions/scope-001.js still fails with the same errors.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The reason that this still fails is that bug 393269 isn't yet checked in. Because XOWs currently fail to propagate information like "is permanent", we end up discarding information in js_SetProperty.
Depends on: 393269
This should be fixed again now that bug 393269 has been checked back in.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
verified fixed 1.9.0
Status: RESOLVED → VERIFIED
It's more likely bug 399587. I'll look into it when I get back from vacationing.
(In reply to comment #17)
> There's a pretty big Private Bytes Regression

Bug 410291
Backed out.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I backed this out in an attempt to fix mochitest orange that appeared when I backed out bug 399587.

The orange was:

*** 10619 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://localhost:8888/tests/content/html/document/test/test_bug255820.html :: runTest :: line 74"  data: no] | got 0, expected 1 | /tests/content/html/document/test/test_bug255820.html
*** 26374 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://localhost:8888/tests/layout/style/test/test_parse_rule.html :: anonymous :: line 221"  data: no] | got 0, expected 1 | /tests/layout/style/test/test_parse_rule.html
That would be expected, see comment 14.
Actually, no. This was never backed out. You *did* back out bug 393269, though. This is FIXED.
Status: REOPENED → RESOLVED
Closed: 17 years ago16 years ago
Resolution: --- → FIXED
verified fixed 1.9.0 again. I'll update public failures as part of the patch for bug 406196.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: