Closed Bug 1135993 Opened 6 years ago Closed 6 years ago

Remove js::IsInNonStrictPropertySet

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jorendorff, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Jeff pointed out in bug 1113369 comment 34 that the bytecode sniffing is gross, and we can now do `return result.failNoNamedSetter();` instead.
Assignee: nobody → evilpies
The error message gets slightly worse, because we don't include the object class.
Attachment #8577221 - Flags: review?(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...)
Attachment #8577221 - Flags: review?(jorendorff) → review+
There is some issue on try with Xrays, will investigate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4427cc42555a
Now it looks like the result isn't always initialized https://treeherder.mozilla.org/#/jobs?repo=try&revision=63eaece532a0
I had to initialize some more ObjectOpResults.
Attachment #8577221 - Attachment is obsolete: true
Attachment #8578594 - Flags: review?(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.
Attachment #8578594 - Flags: review?(jorendorff)
Flags: needinfo?(evilpies)
This is weird enough, so I am going to ask for an additional DOM peer review.
Attachment #8578594 - Attachment is obsolete: true
Attachment #8597448 - Flags: review?(peterv)
Attachment #8597448 - Flags: review?(jorendorff)
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?).
Attachment #8597448 - Flags: review?(peterv) → review+
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).
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...
Blocks: 1139351
Attachment #8597448 - Flags: review?(jorendorff)
I am going to rename *defined to *done in a new patch.
Attachment #8597448 - Attachment is obsolete: true
Attachment #8613119 - Flags: review?(jorendorff)
Comment on attachment 8613119 [details] [diff] [review]
Show better warnings

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

Thanks again.
Attachment #8613119 - Flags: review?(jorendorff) → review+
Blocks: 1170775
https://hg.mozilla.org/mozilla-central/rev/7aaaba95abe8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.