If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Differential Testing: Different output message involving __noSuchMethod__

RESOLVED FIXED in Firefox 28

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gkw, Assigned: djvj)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla28
x86
Windows 7
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox28 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}

shows the following on js opt shell on m-c changeset 8c5a94ba1096 without any CLI arguments:

f
f

However it shows only the following with --ion-eager:

f


More fun when pasted in the shell:

$ ./js-opt-32-dm-ts-windows-8c5a94ba1096.exe
js> try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}
f
f
[(void 0), (void 0)]
js>

$ ./js-opt-32-dm-ts-windows-8c5a94ba1096.exe --ion-eager
js> try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}
f
function print() {
    [native code]
}
js>

My configure flags are:

 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <more NSPR compilation flags>
Flags: needinfo?(jdemooij)
(Reporter)

Updated

4 years ago
Blocks: 465479
(Reporter)

Updated

4 years ago
Whiteboard: [jsbugmon:update]
(Reporter)

Updated

4 years ago
Severity: critical → major
Kannan, can you take this? It looks like the Baseline GetProp IC attaches a stub even if we hit the __noSuchMethod__ case.

What's worse, the GetProp_Native stubs should probably also guard against this in the CALLPROP case, because it's possible to change a property's value from object to primitive without changing the shape :( JM and Ion probably have the same issue... I don't know if fixing that is worth it..

Maybe we can even remove __noSuchMethod__ now, bug 683218?
Flags: needinfo?(jdemooij) → needinfo?(kvijayan)
Same applies to CALLELEM. I'm afraid it will be a lot of work to make this work with the JITs in all cases, especially for indexed properties...
(Assignee)

Updated

4 years ago
Assignee: general → kvijayan
Flags: needinfo?(kvijayan)
(Assignee)

Updated

4 years ago
Depends on: 916949
(Assignee)

Comment 3

4 years ago
Created attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Modifies baseline stub generation for CALLPROP and CALLELEM to ensure that noSuchMethod handling is correct.

In some cases, this is handled by modifying the stubs to explicitly check the result for undefined, and if so, invoke a noSuchMethod handler.  In other cases, this is handled by not generating optimized stubs when the op is CALLPROP or CALLELEM.
(Assignee)

Comment 4

4 years ago
Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

decoder.. can I get this patch fuzzed, in particular exercising code that uses __noSuchMethod___ in various scenarios?

This patch is green on try for linux, linux64, and android.. but since __noSuchMethod__ is super heavily exercised in the test suites, I think a good round of fuzzer coverage would be useful.
Attachment #821810 - Flags: feedback?(choller)
(Assignee)

Updated

4 years ago
Attachment #821810 - Flags: review?(efaustbmo)

Comment 5

4 years ago
Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

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

Looks good. We should make sure that we aren't making a perf gaff with these disablings, but I agree they should be rare. There are a few comments below, and we discussed some of them already on IRC. r=me

::: js/src/jit/BaselineIC.cpp
@@ +3809,5 @@
>          !stub->hasStub(ICStub::GetElem_String))
>      {
> +        // Don't attach this stub for CALLPROP ops which might need noSuchMethod handling.
> +#if JS_HAS_NO_SUCH_METHOD
> +        if (isCallElem)

This is fine, but conservative, as discussed. Not only does a CALLELEM in this case make no semantic sense, but it will never invoke __nSM__ in practice, since the incoming string is not an object to begin with. In general, we should aim to be consistent between GETELEM and GETPROP, so if we are going to leave GetProp_String without a check of this kind, then we should with GETELEM as well.

@@ +4274,5 @@
> +        Address valAddr(scratchReg, 0);
> +
> +        // Check if __noSuchMethod__ needs to be called.
> +#if JS_HAS_NO_SUCH_METHOD
> +        entersStubFrame_ = true;

This looks like it's already set above, regardless of which stub is actually be generated. Line 4148, locally.

@@ +4296,5 @@
> +                return false;
> +            
> +            leaveStubFrame(masm);
> +            // Result is already in R0
> +            masm.jump(&afterNoSuchMethod); 

nit: whitespace at end of line and on line before leaveStubFrame()

@@ +4460,5 @@
> +        regs.takeUnchecked(obj);
> +        regs.take(R1);
> +
> +        masm.pushValue(R1);
> +        masm.loadValue(element, R1);

The first time we check the address, and this time we check the value after we've loaded it. Is there a reason for the difference in style? Perhaps GETPROP vs. GETELEM?

@@ +4474,5 @@
> +            return false;
> +        
> +        leaveStubFrame(masm);
> +        // Result is already in R0
> +        masm.jump(&afterNoSuchMethod); 

Your whitespace betrays your copypasta :P

@@ +4663,3 @@
>      ValueOperand tempVal = regs.takeAnyValue();
>      masm.loadValue(BaseIndex(argData, idxReg, ScaleFromElemWidth(sizeof(Value))), tempVal);
> +    regs.add(argData);

Why is this readded? I understand why it used to be, as we used the register set again later to get a bunch of spare registers. But now, we have line 4678, which starts with a new register set. Am I missing something?

@@ +4689,5 @@
> +            return false;
> +        
> +        leaveStubFrame(masm);
> +        // Result is already in R0
> +        masm.jump(&afterNoSuchMethod); 

whitespace

@@ +6033,5 @@
>      if (cacheableCall && isScripted && !isDOMProxy) {
> +#if JS_HAS_NO_SUCH_METHOD
> +        // It's hard to keep the original object alive through a call, and it's unlikely
> +        // that a getter will be used in callprop locations.  Just don't attach
> +        // stubs in that case.

Can we add a bit to this comment about using getters to generate functions being rare?

@@ +6106,5 @@
>      // If it's a shadowed listbase proxy property, attach stub to call Proxy::get instead.
>      if (isDOMProxy && domProxyShadowsResult == Shadows) {
>          JS_ASSERT(obj == holder);
> +#if JS_HAS_NO_SUCH_METHOD
> +        if (isCallProp)

This one is slightly dubious in it's potential rarity. We should talk to bz, and probably perform some amount of performance testing to ensure that this is not a problem.

@@ +6475,5 @@
> +        masm.pushValue(R1);
> +        masm.push(objReg);
> +        if (!callVM(LookupNoSuchMethodHandlerInfo, masm))
> +            return false;
> +        

nit: whitespace on previous line.

::: js/src/jit/BaselineIC.h
@@ +3136,5 @@
> +#if JS_HAS_NO_SUCH_METHOD
> +        return static_cast<int32_t>(kind) |
> +               (static_cast<int32_t>(isCallElem_) << 16) |
> +               (static_cast<int32_t>(needsAtomize_) << 17) |
> +               (static_cast<int32_t>(acctype_) << 18);

Is there a good reason to make isCallElem_ the first thing here? Why not just stick it at the end, and then we don't need to have a whole different cascade behind the ifdef, just a single binary or. We want it to line up with other stubs, or something?

It makes me really uncomfortable that we have different bit meanings based on an ifdef. How much more churn are we likely to have here? If not much, can we just bubble it to the end in all these stubs?

::: js/src/jit/arm/BaselineHelpers-arm.h
@@ +195,5 @@
>      }
>  }
>  
>  inline void
> +EmitUnstowICValues(MacroAssembler &masm, int values, bool discard=false)

Are these changes used anywhere? I don't see any other references to EmitUnstowICValues in the patch...

I don't particularly object ot them, but we shouldn't add them needlessly.
Attachment #821810 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Eric Faust [:efaust] from comment #5)
> @@ +4460,5 @@
> > +        regs.takeUnchecked(obj);
> > +        regs.take(R1);
> > +
> > +        masm.pushValue(R1);
> > +        masm.loadValue(element, R1);
> 
> The first time we check the address, and this time we check the value after
> we've loaded it. Is there a reason for the difference in style? Perhaps
> GETPROP vs. GETELEM?

It's an |Address| vs. |BaseIndex| thing.  It's possible to type-check an Address-referenced value directly from memory.  On x86 we just add 4 to the offset and read a 32-bit value from there.  For BaseIndex (which are scaled register offset addresses), that's not possible.


> 
> @@ +6106,5 @@
> >      // If it's a shadowed listbase proxy property, attach stub to call Proxy::get instead.
> >      if (isDOMProxy && domProxyShadowsResult == Shadows) {
> >          JS_ASSERT(obj == holder);
> > +#if JS_HAS_NO_SUCH_METHOD
> > +        if (isCallProp)
> 
> This one is slightly dubious in it's potential rarity. We should talk to bz,
> and probably perform some amount of performance testing to ensure that this
> is not a problem.

Agreed.


> ::: js/src/jit/BaselineIC.h
> @@ +3136,5 @@
> > +#if JS_HAS_NO_SUCH_METHOD
> > +        return static_cast<int32_t>(kind) |
> > +               (static_cast<int32_t>(isCallElem_) << 16) |
> > +               (static_cast<int32_t>(needsAtomize_) << 17) |
> > +               (static_cast<int32_t>(acctype_) << 18);
> 
> Is there a good reason to make isCallElem_ the first thing here? Why not
> just stick it at the end, and then we don't need to have a whole different
> cascade behind the ifdef, just a single binary or. We want it to line up
> with other stubs, or something?
> 
> It makes me really uncomfortable that we have different bit meanings based
> on an ifdef. How much more churn are we likely to have here? If not much,
> can we just bubble it to the end in all these stubs?

Couple of reasons:
1. isCallElem seems more 'related' to the kind - a statement about what kind of operation is being run.

2. I find it syntactically ugly to leave a statement unfinished, except an #ifdeffed block finishes the statement, and the #else block of it contains a single semicolon that finishes the statement.  IMHO it messes with readability.  In this case, it's very clear that two different things are happening, in which cases they're happening, and what's happening in each case.

It's unlikely that there will be much churn in these cases.  If you feel strongly about this I can change it to the way you suggest, but I honestly feel that this is more readable.

Comment 7

4 years ago
(In reply to Kannan Vijayan [:djvj] from comment #6)

> It's unlikely that there will be much churn in these cases.  If you feel
> strongly about this I can change it to the way you suggest, but I honestly
> feel that this is more readable.

Nah, you're right. I didn't consider the semicolon. That sounds really ugly.
Depends on: 934956
(Reporter)

Comment 8

4 years ago
Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Not sure if I'll have time for testing this patch anytime soon, but let's see if I can get this on my radar.
Attachment #821810 - Flags: feedback?(gary)
(Reporter)

Comment 9

4 years ago
(In reply to Kannan Vijayan [:djvj] from comment #3)
> Created attachment 821810 [details] [diff] [review]
> baseline-nosuchmethod-handling.patch

fwiw, this patch fails to apply on tip today, so testing using a m-c rev from Oct 28, rev f9fe2c148aa8 (which seems to work).
(Reporter)

Comment 10

4 years ago
Actually, it'll be best to get a rebase on tip - that rev wasn't particularly stable.
Flags: needinfo?(kvijayan)
(Reporter)

Updated

4 years ago
Depends on: 936415
(Assignee)

Comment 11

4 years ago
I just tried to apply to mozilla-inbound tip and it applies fine.  Just tried on recent pull of m-c and it applies cleanly with some offsets.

What rev are you trying to apply it to?  Have you tried the latest tip as of today?
Flags: needinfo?(kvijayan)
(Reporter)

Comment 12

4 years ago
(on m-c tip rev a07aebef20e7)

$ patch -p1 --dry-run < ~/Desktop/912303-c3.diff 
patching file js/src/jit/BaselineIC.cpp
Hunk #1 succeeded at 3483 (offset -13 lines).
Hunk #2 succeeded at 3675 (offset -13 lines).
Hunk #3 FAILED at 3719.
Hunk #4 succeeded at 3752 (offset -13 lines).
Hunk #5 succeeded at 3777 (offset -13 lines).
Hunk #6 succeeded at 3830 (offset -13 lines).
Hunk #7 succeeded at 3865 (offset -13 lines).
Hunk #8 succeeded at 4237 (offset -13 lines).
Hunk #9 succeeded at 4424 (offset -13 lines).
Hunk #10 succeeded at 4506 (offset -13 lines).
Hunk #11 succeeded at 4635 (offset -13 lines).
Hunk #12 FAILED at 5985.
Hunk #13 succeeded at 6021 (offset -15 lines).
Hunk #14 succeeded at 6068 (offset -15 lines).
Hunk #15 succeeded at 6089 (offset -15 lines).
Hunk #16 succeeded at 6200 (offset -15 lines).
Hunk #17 succeeded at 6365 (offset -15 lines).
Hunk #18 succeeded at 6387 (offset -15 lines).
Hunk #19 succeeded at 6407 (offset -15 lines).
Hunk #20 succeeded at 6553 (offset -15 lines).
Hunk #21 succeeded at 6626 (offset -15 lines).
Hunk #22 succeeded at 6718 (offset -15 lines).
Hunk #23 succeeded at 6871 (offset -15 lines).
2 out of 23 hunks FAILED -- saving rejects to file js/src/jit/BaselineIC.cpp.rej
patching file js/src/jit/BaselineIC.h
patching file js/src/jit/arm/BaselineHelpers-arm.h
patching file js/src/jit/arm/MacroAssembler-arm.cpp
patching file js/src/jit/arm/MacroAssembler-arm.h
patching file js/src/jit/x64/BaselineHelpers-x64.h
patching file js/src/jit/x64/MacroAssembler-x64.h
Hunk #1 succeeded at 351 (offset 1 line).
Hunk #2 succeeded at 806 (offset 2 lines).
patching file js/src/jit/x86/BaselineHelpers-x86.h
patching file js/src/jit/x86/MacroAssembler-x86.h
Hunk #1 succeeded at 352 (offset 1 line).
Flags: needinfo?(kvijayan)
Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Haven't spotted anything going wrong here.
Attachment #821810 - Flags: feedback?(choller) → feedback+
(Assignee)

Comment 14

4 years ago
(In reply to Gary Kwong [:gkw] [:nth10sd] (yes, still catching up on bugmail) from comment #12)
> (on m-c tip rev a07aebef20e7)
> 
> $ patch -p1 --dry-run < ~/Desktop/912303-c3.diff 
> patching file js/src/jit/BaselineIC.cpp
> Hunk #1 succeeded at 3483 (offset -13 lines).
...

This is really weird.  Are you sure you have the right diff?

Regardless, I'm going to try to land the bug now and see how it goes.  It's coming up green on try and seems OK with :decoder's fuzzing, so it should be fine.
Flags: needinfo?(kvijayan)
(Reporter)

Comment 15

4 years ago
Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Strange that the patch application failed for me. Your suggested way of moving forward works, too. I'll be sure to file new bugs as they come along.
Attachment #821810 - Flags: feedback?(gary)
(Assignee)

Comment 16

4 years ago
That'll be appreciated, thanks Gary!  If you see noSuchMethod crashes going forward it might be worthwhile to test this commit for regression.

https://hg.mozilla.org/integration/mozilla-inbound/rev/58605e9a6ea1
https://hg.mozilla.org/mozilla-central/rev/58605e9a6ea1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Reporter)

Updated

4 years ago
Depends on: 938130
(Reporter)

Updated

4 years ago
Depends on: 940846
Depends on: 941612
(Reporter)

Updated

4 years ago
Depends on: 945223
status-firefox28: --- → fixed
Flags: in-testsuite?
(Reporter)

Updated

3 years ago
Depends on: 1030460
You need to log in before you can comment on or make changes to this bug.