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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bc, Assigned: mrbkap)
References
()
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
2.07 KB,
patch
|
jst
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
Details | Diff | Splinter Review |
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+
Reporter | ||
Updated•17 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 1•17 years ago
|
||
I think this should fix the bug -- I'll compile and test in a bit.
Assignee: crowder → mrbkap
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•17 years ago
|
||
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
Comment 3•17 years ago
|
||
Are this bug and bug 393269 dupable, then? Does that patch still make this problem -worse-?
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Attachment #287959 -
Flags: review? → review?(jst)
Comment 7•17 years ago
|
||
Comment on attachment 287959 [details] [diff] [review] Ugly brutal fix Fun.
Attachment #287959 -
Flags: review?(jst) → review+
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 8•17 years ago
|
||
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+
Comment 9•17 years ago
|
||
do we want to check this in and close it?
Reporter | ||
Comment 10•17 years ago
|
||
mrbkap, can we get this in ?
Comment 11•17 years ago
|
||
I just landed this, with an updated comment. Blake, please have a look at the new comment and verify that it still reflects reality :)
Comment 12•17 years ago
|
||
Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 13•17 years ago
|
||
js1_5/extensions/scope-001.js still fails with the same errors.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•17 years ago
|
||
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
Assignee | ||
Comment 15•17 years ago
|
||
This should be fixed again now that bug 393269 has been checked back in.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
There's a pretty big Private Bytes Regression http://graphs.mozilla.org/#spst=range&spss=1198976687.2064924&spse=1199005000.5734897&spstart=1196883676&spend=1199299024&bpst=cursor&bpsc=1198967975.4012623&bpstart=1198976687.2064924&bpend=1199005000.5734897&m1tid=53258&m1bl=0&m1avg=0 This and bug 399587 are the only checkins during that time. Possible this increased memory usage?
Assignee | ||
Comment 18•17 years ago
|
||
It's more likely bug 399587. I'll look into it when I get back from vacationing.
Comment 19•17 years ago
|
||
(In reply to comment #17) > There's a pretty big Private Bytes Regression Bug 410291
Comment 21•17 years ago
|
||
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
Assignee | ||
Comment 22•17 years ago
|
||
That would be expected, see comment 14.
Assignee | ||
Comment 23•16 years ago
|
||
Actually, no. This was never backed out. You *did* back out bug 393269, though. This is FIXED.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 24•16 years ago
|
||
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.
Description
•