825379 - IonMonkey: Differential Testing: Getting different output w/without --ion-eager with forEach

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: regressing changeset range

x = this
y = {}
y.v = ""
try {
    [0, 0].forEach(function() {
        for (p in y) {
            x.eval("function f(){yield e}")
        }
    })
    print('FOO')
} catch (e) {}

shows the following output in js debug and opt shell on m-c changeset 0bb4773db082 without --ion-eager:

FOO

but does not show anything with --ion-eager.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Easy two-line fix, findVersion() uses cx->fp() and this breaks with Ion. No UnrootedScript because we want to move to a raw JSScript * again.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d0d20fbaf1

Comment 4

[Daniel Holbert [:dholbert]]

m-i has had a perma-orange ever since this landed in this push:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=06d0d20fbaf1

Of the three things in that push: one is an empty merge commit, the second is a backout-commit that landed on m-c as well (and didn't cause orange there), and the third is this bug's patch. So, it looks likely that this bug's patch is what's causing the perma-orange.

Comment 5

[Daniel Holbert [:dholbert]]

kneejerk backout, per comment 4:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b66caf705f6

Comment 6

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

jandem, thoughts?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] from comment #6)
> jandem, thoughts?

Hm, the cause is not obvious from the logs. I can take a closer look tomorrow.

(And btw, can you ask me next time before pushing one of my patches? It's not a big deal but I'd have sent this one to try first.)

Comment 8

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

> (And btw, can you ask me next time before pushing one of my patches? It's
> not a big deal but I'd have sent this one to try first.)

Sure - though I set checkin-needed I didn't really expect it to be landed that quickly.

Sorry if I stepped on your toes and/or jumped the gun too quickly there, there was no indication of a definite need for a prior try run.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Gary Kwong [:gkw] from comment #8)
> Sorry if I stepped on your toes and/or jumped the gun too quickly there,
> there was no indication of a definite need for a prior try run.

(Generally, I'd recommend letting the patch-author be the one to set checkin-needed (and/or do the checkin), in case they happen to have run across issues w/ testing, or have any side-channel review comments to fix up before landing, etc.)

Comment 10

[Jeff Walden [:Waldo]]

Yeah, I'd agree with comment 9.  Unless (possibly non-exhaustively, just covering the reasons that spring immediately to mind):

* there's feedback from higher-up that something must land ASAP (say, something's b2g-prioritized),
* there's lateral feedback that a patch is really needed for some reason (someone's patchwork depends on it landing, and it'll substantially help them to land it) (or not *needed* so much but very *wanted*, and simple enough to boot -- I'm thinking of reviewed warning-fixes when I think of this category), or
* you think it'll avoid exposing nightly users to a widespread common issue

...there's no reason to reason to guess at the next step for a patch.

Comment 11

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

> * there's lateral feedback that a patch is really needed for some reason
> (someone's patchwork depends on it landing, and it'll substantially help
> them to land it) (or not *needed* so much but very *wanted*, and simple
> enough to boot -- I'm thinking of reviewed warning-fixes when I think of
> this category), or

Well, it does help with fuzzing, but I guess the sentiment is that it does not seem important enough here unless one asks, which on hindsight I probably should have (and I am, moving forward).

(Historically though, there have been patches that were r+'ed that just lay around unlanded for quite a few weeks due to it slipping through the cracks.

It gets all the more *frustrating* when the patch author then becomes unavailable for various reasons (e.g. holidays, etc.), and when subsequent bugs are found that turn out to be dupes that this patch fixes, thus wasting reduction & QA time.

But this discussion should probably happen away from this bug)

Comment 12

[Jan de Mooij [:jandem]]

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

(Try: https://tbpl.mozilla.org/?tree=Try&rev=960b4df11b09)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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