428021 - Can't define both a getter and a setter for a property of the global object

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Adam Kowalczyk]

It looks like the fix for bug 420585 wasn't complete. Defining either a getter or a setter works fine, but when attempting to define both of them, only the first one is registered.

See the attached testcase - setting |x| throws an exception as if there was no setter.

Comment 1

[Adam Kowalczyk]

Attachment: testcase

Comment 2

[Damon Sicore (:damons)]

Why should this stop ship?

Comment 3

[Adam Kowalczyk]

I've nominated it, because it's very similar to bug 420585 which was marked as a blocker by Brendan. I guess it breaks a semi-major JS feature.

Comment 4

[Brendan Eich [:brendan]]

Yeah, we fixed half-duplex situations, we should fix the full thing. Blake, can you take this bug?

/be

Comment 5

[Damon Sicore (:damons)]

Blocking per Comment #4.

Comment 6

[Damon Sicore (:damons)]

Crowder?  How are we doing here?

Comment 7

[Brian Crowder]

Attachment: can't short-circuit as much as we once did

The "goto out" here was bogus, we still need to run the rest of the routine here so that the clasp->addProperty routine still happens.  Without that, XOWs can't propagate to the wrapped object.

Comment 8

[Brian Crowder]

Comment on attachment 317055 [details] [diff] [review]
can't short-circuit as much as we once did

Some muck-up with my test-rig caused ALL my tests to fail for both baseline and patched tests, so I missed some regressions caused by this patch.  Needs more work.

Comment 9

[Brian Crowder]

Attachment: closer to working

Comment 10

[Brian Crowder]

Attachment: passes js/tests suite

Going to check this against sunspider, now.

Comment 11

[Brian Crowder]

Comment on attachment 317118 [details] [diff] [review]
passes js/tests suite

I think that this will slightly regress sunspider (by fractions of a percent), perhaps moreso on browser-builds, but it is needed for correctness.

Comment 12

[Brian Crowder]

Attachment: addressing shaver's review from IRC

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 317327 [details] [diff] [review]
addressing shaver's review from IRC

r+a=shaver

Comment 14

[Brian Crowder]

Attachment: a mochitest for this bug

The testcase (plus a couple additional checks) from attachment 314615 [details] as a mochitest.

Comment 15

[Brian Crowder]

Without the patch from comment 12, two of the three checks added in this mochitest fail (the getter still runs).

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Would it make sense to stick the new mochitest in js/src/xpconnect/tests/mochitest next to test_wrappers.html?

Comment 17

[Brian Crowder]

Blake, works for me; I really wasn't entirely sure -where- to put this.  New patch with the relocated test coming right up.

Comment 18

[Brian Crowder]

Attachment: relocated

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 317358 [details] [diff] [review]
relocated

>+      var exn = false;
>+      var e;
>+      try {
>+          e = x;
>+          x = false;
>+      } catch (e) {
>+          exn = true;
>+      }
>+      ok(!exn, "Exception caught: " + e);

Er, this |e| isn't the exception. Why not just fail inside the catch block (you don't even need |exn| then)?

r=mrbkap with that fixed.

Comment 20

[Brian Crowder]

Attachment: patch to land

Rolling up the patch and the testcase together.  mrbkap, please r? the testcase again -- feel free to eyeball the code, too, if you like.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 317383 [details] [diff] [review]
patch to land

>+          ok(0, "Exception caught: " + e);

Just use |fail("Exception caught: " + e);|?

Comment 22

[Brian Crowder]

Attachment: patch to land, for real

closer to what mrbkap likes, but with a consistent test count.

Comment 23

[Brian Crowder]

"patch to land, for real" has r+/a+ on the parts that need approval, and r+ on the tests, please land with a note attributing review to shaver and mrbkap.  If this doesn't land this afternoon, I will land it tonight.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

mozilla/js/src/jsobj.c 	3.468
mozilla/js/src/xpconnect/tests/mochitest/Makefile.in 	1.8
mozilla/js/src/xpconnect/tests/mochitest/test_bug428021.html 	1.1

Comment 25

[Bob Clary [:bc] (inactive)]

v on centos,win2k3 tinderboxen