620141 - eval cache should key based on calling script, not calling function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The eval cache keys on calling function to ensure proper strict mode matching.  But this is wrong in the case where the calling frame is an eval frame in strict mode, called directly from a function frame which is *not* in strict mode.  That's bug 620130, which will be minimally hacked around to not cache evals in eval code that happens to be from a function frame.  But the better fix would be to cache eval code based on the calling script.

Comment 1

[Jason Orendorff [:jorendorff]]

It's not obvious to me that this code is even correct. We match eight things, the last of which is:

                     * [...] qualify by comparing scopeobj to the
                     * COMPILE_N_GO-memoized parent of the first literal
                     * function or regexp object if any. If none, then this
                     * script has no compiled-in dependencies on the prior
                     * eval's scopeobj.

I can't get it to break, but I am worried, because you can have stuff like this:

    let (x = 1)
        a = eval(s);
    let (y = 2)
        b = eval(s);

where we do in fact use the same eval script in two different lexical scopes!

In that light, re-read the comment. I don't know if there are other compiled-in dependencies or not. But we have to worry about not-compiled-in dependencies too. What about plain old JSOP_NAME? Doesn't the property cache assume anything about lexical scopes being the same at the same PC? What about PICs?

So in short, yeah, matching on the calling JSScript * plus maybe the pc would be much more obviously-correct.

Comment 2

[Jason Orendorff [:jorendorff]]

Heh, credit where due: Jim Blandy noticed the scariness of that matching code (using brain power alone, not even *looking* at the scary code, which was on *my* screen :).

Comment 3

[Usman]

Hi, I'd like to try work on this bug.
Can this bug be assigned to me please?

Comment 4

[Usman]

(In reply to Usman from comment #3)
> Hi, I'd like to try work on this bug.
> Can this bug be assigned to me please?

I'm new to Mozilla development, I have built Firefox nightlies, and working on learning how to use the Javascript shell to replicate and fix this bug.

Thanks

Comment 5

[Jeff Walden [:Waldo]]

Sure, done.  Feel free to ask any questions you might have.  Also feel free to join the #jsapi IRC channel on irc.mozilla.org -- the JS team hangs out and discusses things there, and even just osmosing is a good way to learn more.

Comment 6

[Usman]

Resetting assignee - back to the list - Ive held this for too long with no outcome.
I'm still having trouble getting the jstests.py to complete on my spidermonkey build.
Will come back when I know more.

Thanks.

Comment 7

[Philipp Matthias Schäfer [:fiveop]]

Attachment: patch requesting feedback / review

I wrote this (first) patch guided by jorendorff. Next to hoping it solves the problem, I am mostly concerned with having failed to follow whatever best practice rules there are (mainly naming conventions and usage of Handle, Rootet, Raw, etc. wrappers).

The changes compile and pass all jit-test tests. If I should have run other tests as well, please tell me.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 719167 [details] [diff] [review]
patch requesting feedback / review

@Philipp: You can set the review or the feedback flag to the bugzilla email address of the person you want to do the review/ give feedback. This way, they get notified.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 719167 [details] [diff] [review]
patch requesting feedback / review

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

For a while I wasn't sure how much of this patch we should take.

I did a little more research today into the reasons we don't have a major horrifying bug here, and I'm totally convinced, we should take the whole thing. What we've got right now is a house of cards. I think if Ion was just a little bit smarter we'd be doomed.

I don't see anything wrong with this patch at all; the only thing I'm going to change is to rename EvalCacheObject to EvalCacheEntry (we tend to reserve "Object" for JS objects).

I'll land this, probably not today.

Comment 10

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47495d62d7f7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47495d62d7f7