1081850 - Differential Testing: Different output message involving prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    function f() {
        print(eval.prototype)
    }
    f()
    function eval()(0)
    f()
} catch (e) {}

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-f547cf19d104 --fuzzing-safe --no-threads --ion-eager testcase.js
undefined
undefined

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-f547cf19d104 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
undefined
[object Object]

Tested this on m-c rev f547cf19d104.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This seems to go back beyond e8558ecd9b16.

Jan, any idea what's up here?

Comment 1

[Jan de Mooij [:jandem]]

Looks like a Baseline regression from bug 928928. It works if I make TryAttachNativeDoesNotExistStub return early.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: bug-1081850.patch

Was not checking for class resolve hooks when looking for no-such-property operations to optimize.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8520884 [details] [diff] [review]
bug-1081850.patch

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

Looks good; please add a testcase like the one Gary posted in comment 0 that fails without the patch.

Comment 4

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c785a79ee9

Pushed with test case.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3c785a79ee9

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 8520884 [details] [diff] [review]
bug-1081850.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 928928.

[User impact if declined]:
Wrong JS execution semantics in the wild.

[Describe test coverage new/current, TBPL]:
Added test to cover case (see commit e3c785a79ee9)

[Risks and why]:
Low risk.  Disables baseline-jit optimization of particular cases, adds no other new logic.

[String/UUID change made/needed]:
None.

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 8520884 [details] [diff] [review]
bug-1081850.patch

The offending patch landed in June 11.  The bug has made it into beta.

[Feature/regressing bug #]:
Bug 928928.

[User impact if declined]:
Wrong JS execution semantics in the wild.

[Describe test coverage new/current, TBPL]:
Added test to cover case (see commit e3c785a79ee9)

[Risks and why]:
Low risk.  Disables baseline-jit optimization of particular cases, adds no other new logic.

[String/UUID change made/needed]:
None.

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Kannan Vijayan [:djvj] from comment #7)
> Comment on attachment 8520884 [details] [diff] [review]
> bug-1081850.patch
> 
> The offending patch landed in June 11.  The bug has made it into beta.

According to bug 928928, this bug shipped in Firefox 33.

> [User impact if declined]:
> Wrong JS execution semantics in the wild.

Is this visible to the user? If so, how does this bug impact the user and do you have a concrete error case?

Comment 9

[Kannan Vijayan [:djvj]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> (In reply to Kannan Vijayan [:djvj] from comment #7)
> > Comment on attachment 8520884 [details] [diff] [review]
> > bug-1081850.patch
> > 
> > The offending patch landed in June 11.  The bug has made it into beta.
> 
> According to bug 928928, this bug shipped in Firefox 33.

Should I be asking for uplift to release?
 
> > [User impact if declined]:
> > Wrong JS execution semantics in the wild.
> 
> Is this visible to the user? If so, how does this bug impact the user and do
> you have a concrete error case?

It seems that bug 1092204 is a dup of this one - that's a bug reporting broken pages on a private site.  I'm waiting for confirmation from reporter that it has been fixed before marking as a dup, but I suspect it'll come back positive.

It seems serious enough to uplift to beta at least.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8520884 [details] [diff] [review]
bug-1081850.patch

(In reply to Kannan Vijayan [:djvj] from comment #9)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> > (In reply to Kannan Vijayan [:djvj] from comment #7)
> > > Comment on attachment 8520884 [details] [diff] [review]
> > > bug-1081850.patch
> > > 
> > > The offending patch landed in June 11.  The bug has made it into beta.
> > 
> > According to bug 928928, this bug shipped in Firefox 33.
> 
> Should I be asking for uplift to release?

No. Given how close we are to 34 I don't think this bug warrants a point release.

> > > [User impact if declined]:
> > > Wrong JS execution semantics in the wild.
> > 
> > Is this visible to the user? If so, how does this bug impact the user and do
> > you have a concrete error case?
> 
> It seems that bug 1092204 is a dup of this one - that's a bug reporting
> broken pages on a private site.  I'm waiting for confirmation from reporter
> that it has been fixed before marking as a dup, but I suspect it'll come
> back positive.
> 
> It seems serious enough to uplift to beta at least.

Thank you for the bug reference. I agree. Beta+

Can you please land this on Beta yourself as I think we don't have sheriff coverage right now and really need the fix in before 2pm PT.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b747cb4b544
https://hg.mozilla.org/releases/mozilla-beta/rev/e685be9bd4d6

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8520884 [details] [diff] [review]
bug-1081850.patch

Aurora+

Comment 13

[Kannan Vijayan [:djvj]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #10)
> Thank you for the bug reference. I agree. Beta+
> 
> Can you please land this on Beta yourself as I think we don't have sheriff
> coverage right now and really need the fix in before 2pm PT.

Landing now.  Haven't landed on beta before so I need to pull the repos first.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e685be9bd4d6