1760844 - Consider removing hasInstance proxy trap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jan de Mooij [:jandem]]

Bug 1270746 removed the hasInstance JSClass hook, but we still have the non-standard hasInstance proxy trap. This trap makes it harder to optimize instanceof, because it means we can't easily desugar instanceof in the bytecode emitter for example.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP: Bug 1760844 - WIP: remove hasInstance proxy trap.

Comment 2

[Jan de Mooij [:jandem]]

The attached WIP patch is mostly green on try, but it exposes an issue with Xray waivers.

The behavior implemented in bug 1504660 is that xrayWrapper instanceof Cu.waiveXrays(..) returns false.

If we remove the hasInstance proxy trap, this changes: we now look up Symbol.hasInstance on the RHS object (the waiver) and call the hasInstance function that returns. See the changes to test_xray_instanceof.js in the WIP patch (I only made the changes necessary to make the test pass).

bholley/robwu: I see there was a lot of discussion in bug 1504660. Do you have any thoughts on this? Is a change in behavior here acceptable?

Comment 3

[Bobby Holley (:bholley)]

I think it's acceptable. IIRC the problem we were trying to solve that |RHS instanceof LHS| could enter the compartment of LHS even if LHS was an XRayWrapper (to a scripted proxy). Entering the compartment is a more reasonable thing to do if RHS is a waiver. I don't fully recall why we made the waiver instanceof behavior match the Xray instanceof behavior in this case, but I suspect it was just easier to implement.

Rob, does that sound right to you?

Note that you'll also want to update the strings in the relevant tests.

Comment 4

[Rob Wu [:robwu]]

The new behavior is reasonable, but the messages that describe the test expectations should be updated to match the new behavior.

The original patch was meant to handle the scenario described at https://bugzilla.mozilla.org/show_bug.cgi?id=1504660#c14.

I implemented the current behavior because I had to choose a behavior, and evaluating to false is a safe default choice for mismatching wrappers.
One could argue that if the RHS is a waived XrayWrapper (in LHS instanceof RHS), that it's fine to trigger the underlying proxy traps if any. Anyone who expected sane behavior shouldn't have waived Xrays on the RHS.

With the proposed change here, I think that you can also undo the change here: https://hg.mozilla.org/mozilla-central/rev/c50c098e40c4#l9.1

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1760844 part 1 - Remove hasInstance proxy trap. r?iain!,peterv!,robwu!

This removes the non-standard hasInstance proxy trap.

All objects will now use the algorithm in JS::InstanceofOperator. That function
uses other primitives and proxy traps already available to JS code.

As discussed in the bug, there's a minor difference in behavior: for
xrayWrapper instanceof xrayWaiver we used to always return false but we can now
invoke xrayWaiver[Symbol.hasInstance]().

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1760844 part 2 - Tidy up InstanceofOperator and HasInstance. r?iain!

• js::HasInstance now always forwards to JS::InstanceofOperator so call the latter directly.
• Move InstanceofOperator to the js namespace and remove it from the API (JS_HasInstance already exists).

Depends on D142061

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34f5687c0372
part 1 - Remove hasInstance proxy trap. r=iain,peterv,robwu
https://hg.mozilla.org/integration/autoland/rev/ff1e54abeb67
part 2 - Tidy up InstanceofOperator and HasInstance. r=iain

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/34f5687c0372
https://hg.mozilla.org/mozilla-central/rev/ff1e54abeb67