1135993 - Remove js::IsInNonStrictPropertySet

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Orendorff [:jorendorff]]

Jeff pointed out in bug 1113369 comment 34 that the bytecode sniffing is gross, and we can now do `return result.failNoNamedSetter();` instead.

Comment 1

[Tom S [:evilpie]]

Attachment: v1 - Remove js::IsInNonStrictPropertySet

The error message gets slightly worse, because we don't include the object class.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8577221 [details] [diff] [review]
v1 - Remove js::IsInNonStrictPropertySet

Review of attachment 8577221 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too worried about the error messages getting a bit worse; at least the error will be thrown in the right situations!

And if we want these messages to be better, it's an easy fix. (I think I even filed a bug about it...)

Comment 3

[Tom S [:evilpie]]

There is some issue on try with Xrays, will investigate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4427cc42555a

Comment 4

[Tom S [:evilpie]]

Now it looks like the result isn't always initialized https://treeherder.mozilla.org/#/jobs?repo=try&revision=63eaece532a0

Comment 5

[Tom S [:evilpie]]

Attachment: v2 - Remove js::IsInNonStrictPropertySet

I had to initialize some more ObjectOpResults.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8578594 [details] [diff] [review]
v2 - Remove js::IsInNonStrictPropertySet

Review of attachment 8578594 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh. Was tempted to bump this to bz, who actually understands what is supposed to be initialized in which cases here. But his time is a lot more valuable than mine so I went and looked it up.

The complete contract of these Xray traits defineProperty methods is specified by the one caller, XrayWrapper<Base, Traits>::defineProperty(). It says:

    bool defined = false;
    if (!Traits::singleton.defineProperty(cx, wrapper, id, desc, existing_desc, result, &defined))
        return false;
    if (defined)
        return true;
    ...bunch more code...
    return JS_DefinePropertyById(cx, expandoObject, id, wrappedDesc, result);

That is:
*   defined is initialized to false before Traits::defineProperty is called
*   if Traits::defineProperty returns false, it doesn't matter what out parameters were initialized
*   if Traits::defineProperty sets *defined to true and returns true, then it must make sure result is initialized
*   otherwise it should *not* initialize result, so that we will detect the bug if JS_DefinePropertyById fails to initialize it!

Clearing r?. The correct fix here is:
1) go back to version 1 of this patch
2) add `*defined = true;` before `return result.failXxxx();` in Codegen.py, two places.

And now that you understand the contract, assuming you do :), feel free to change it to be less horrible! ---in another bug, please.

Comment 7

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=662ebb55fe1e

Comment 8

[Tom S [:evilpie]]

Attachment: v3 - v1 again with jorendorff's suggestions

This is weird enough, so I am going to ask for an additional DOM peer review.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 8597448 [details] [diff] [review]
v3 - v1 again with jorendorff's suggestions

Review of attachment 8597448 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. Please do file that bug to improve the contract. We might be able to use ObjectOpResult instead of the defined argument (though we'dt need a way to check for Uninitialized?).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fwiw, I think the worse error reporting is a real problem here: it makes the error messages nearly non-actionable.

See comments and needinfo request in bug 1139351 (which has pretty much a superset of this patch, but is blocked on not breaking the error messages).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and as far as the Xrays defined stuff...

I propose we just rename "defined" to "done" and keep everything else as-is.  In particular, it makes total sense to do *done = true if we plan to fail the opresult.

Or we could even just expose an accessor on ObjectOpResult to see whether it's initialized and just use _that_ state in Xrays to decide whether to bail out: if the callee initialized the ObjectOpResult, we're done...

Comment 12

[Tom S [:evilpie]]

Attachment: Show better warnings

I am going to rename *defined to *done in a new patch.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8613119 [details] [diff] [review]
Show better warnings

Review of attachment 8613119 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaaba95abe8

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7aaaba95abe8