619713 - [SeaMonkey] mochitest-browser-chrome: 192 failures due to not supporting 'let' anymore/again

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Serge Gautherie (:sgautherie)]

Last good:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292429880.1292433131.9788.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2010/12/15 08:18:00
rev:f9bf165e0c61
moz:90b17476216d

Intermediate red, but still good:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292452743.1292458606.32655.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2010/12/15 14:39:03
rev:f9bf165e0c61
moz:182c7b6acb30
{
[[e-mail] - 2010/12/15 16:35:44]
possible breakage due to
updating buildbot configurations

TEST-START | chrome://mochitests/content/browser/docshell/test/browser/browser_bug554155.js
TEST-PASS | chrome://mochitests/content/browser/docshell/test/browser/browser_bug554155.js | pushState should cause exactly one LocationChange event.
INFO TEST-END | chrome://mochitests/content/browser/docshell/test/browser/browser_bug554155.js | finished in 801ms
}

New bad:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292478734.1292482792.17583.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2010/12/15 21:52:14
rev:52207f8b382e
moz:e435c9812855
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_bug554155.js | Exception thrown at chrome://mochitests/content/browser/docshell/test/browser/browser_bug554155.js:4 - SyntaxError: missing ; before statement
}
and 191 other failures...

***

s/let/var/g in that test works around this regression: as if bug 597811 fix regressed.

Comment 1

[Serge Gautherie (:sgautherie)]

Good:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292445468.1292449153.17448.gz
Linux comm-central-trunk debug test mochitest-other on 2010/12/15 12:37:48
rev:f9bf165e0c61
moz:baee733bafd8
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292439119.1292440897.11215.gz
OS X 10.5 comm-central-trunk debug test mochitest-other on 2010/12/15 10:51:59
rev:f9bf165e0c61
moz:68529f865a6e
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292436229.1292437840.29438.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2010/12/15 10:03:49
rev:f9bf165e0c61
moz:68529f865a6e

Regressed:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292460429.1292463259.23676.gz
Linux comm-central-trunk debug test mochitest-other on 2010/12/15 16:47:09
rev:f9bf165e0c61
moz:a9928aa3aa68
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292457842.1292459251.3509.gz
OS X 10.5 comm-central-trunk debug test mochitest-other on 2010/12/15 16:04:02
rev:f9bf165e0c61
moz:8755d308796d
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292450598.1292451460.29030.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2010/12/15 14:03:18
rev:f9bf165e0c61
moz:8755d308796d

Comment 2

[Serge Gautherie (:sgautherie)]

Regression timeframe:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=baee733bafd8&tochange=8755d308796d
Nothing obvious in this list.

NB: I think some SeaMonkey buildbot updates (and possibly build clobbers) happened +/- at that time, but I don't imagine they could cause/reveal this, could they?

Comment 3

[Serge Gautherie (:sgautherie)]

Ah, ftr,
bug 597811 fixed mozJSComponentLoader.cpp,
in this new bug, the issue is in the tests themselves (not in a component anymore):
then, either "let" shouldn't be used in tests, or the JS version needs to be forced on tests too!?

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> forced on tests too!?

Maybe something related to the 'mochitest-browser-chrome' harness?

***

Fwiw, I reproduced on my local Windows, with a downloaded build.

Comment 5

[Robert Kaiser]

I'm seeing this locally as well on tests that worked before, so it doesn't look like it's a buildbot harness thing - did we have any comm-central changes in that timeframe?
We're seeing this on all platforms, btw.

Comment 6

[Robert Kaiser]

It actually may have broken on a clobber, so something before this range might be the actual cause.

Comment 7

[David Mandelin [:dmandelin]]

I'm looking at this now. So far I've reproduced it with a nightly, and verified that 'let' requires version 1.7 or higher. I guess I'll just have to debug what versions are getting used and see if I can figure out what's missing or not working.

But if anyone can tell me anything how the test harness (or whatever components are of interest) try to set the version and how it's supposed to work, that would probably help.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

This reminds me of the problem I discovered in bug 606251 comment 1 - various things can cause a JSContext's version to change during execution, and that can lead to all sorts of issues. Bug 595691 was supposed to reduce the complexity and ideally solve those problems...

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
> This reminds me of the problem I discovered in bug 606251 comment 1 - various
> things can cause a JSContext's version to change during execution, and that can
> lead to all sorts of issues.

Yes, it's something like that. In the first failure of this set, we are loading something with |loadSubScript| and it gets loaded with the default version instead of 185. |loadSubScript| just takes cx->findVersion() and uses that. In this case, |loadSubScript| is called from a version 185 script, so ordinarily, the new script would get loaded with 185 and it would work. But something has overridden the version in the JSContext to the default version, which is "sticky" (i.e., cx->findVersion() returns the override version).

Comment 10

[Robert Kaiser]

Hmm, we tried hard some time ago to get all chrome JS run with the most-current version, so it would be good if the same happens here (actually, I'd expect loading a subscript to just get the same version as the caller).

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #10)
> Hmm, we tried hard some time ago to get all chrome JS run with the most-current
> version, so it would be good if the same happens here (actually, I'd expect
> loading a subscript to just get the same version as the caller).

Does that mean it would be correct to change loadSubscript so that it uses the version of the script that called it, and not any override version set in the context? Because that would be an easy patch to write, and it should fix this problem.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> > Hmm, we tried hard some time ago to get all chrome JS run with the most-current
> > version, so it would be good if the same happens here (actually, I'd expect
> > loading a subscript to just get the same version as the caller).
> 
> Does that mean it would be correct to change loadSubscript so that it uses the
> version of the script that called it, and not any override version set in the
> context? Because that would be an easy patch to write, and it should fix this
> problem.

Yes, sorry I didn't see this. The old code did not change version or options at all, it just used the calling xpconnect nsAXPCNativeCallContext's JSContext.

/be

Comment 13

[David Mandelin [:dmandelin]]

Attachment: WIP

This is a patch that makes the subscript load with the same version as the script that loads it. It seems to work, and to fix the problem, but it causes one xpcshell test failure in _tests/xpcshell/js/src/xpconnect/tests/unit/test_bug596580.js:

  scriptLoader.loadSubScript(uri.spec);
  version(150)
  try {
      scriptLoader.loadSubScript(uri.spec);
      throw new Error("Subscript should fail to load.");
  } catch (e if e instanceof SyntaxError) {
      // Okay.
  }

With the patch, the script does load, because it uses the version of the loading script.

So, does that mean this test no longer applies, or does this mean this approach won't work?

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 500923 [details] [diff] [review]
WIP

>      * - The default verion.
and
>+     * - The default verion.

Nit: s/verion/version/

Comment 15

[Robert Kaiser]

(In reply to comment #13)
> So, does that mean this test no longer applies, or does this mean this approach
> won't work?

I guess we'll need cdleary, who worked on the bug this tests, or brendan to answer that question...

Comment 16

[David Mandelin [:dmandelin]]

(In reply to comment #15)
> (In reply to comment #13)
> > So, does that mean this test no longer applies, or does this mean this approach
> > won't work?
> 
> I guess we'll need cdleary, who worked on the bug this tests, or brendan to
> answer that question...

Brendan told me in person that the test no longer applies. I just haven't had time yet to update the patch. I should get it today.

Comment 17

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 18

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> Brendan told me in person that the test no longer applies. I just haven't had
> time yet to update the patch. I should get it today.

NB: Instead of deleting the test, just swap the expected results and probably rename the file ;-)

Comment 20

[David Mandelin [:dmandelin]]

Update: Chris Leary is back, and he says bug 595691 is almost done (ran on try yesterday) and will solve all these problems, so I'm going to wait for that.

Comment 21

[David Mandelin [:dmandelin]]

Flipping this back to Chris because his current work will fix it.

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

I'd be interested in early feedback as to whether the patch queue from bug 595691 fixes this issue. I tried running mochitest-chrome for the seamonkey build locally on my linux machine in hopes of creating a reduced test case but didn't see any failures, so I assume I was just Doing It Wrong.

There are linux 32/64 builds attached to try server changeset a2e24df0e2ec if you can grab it off the ftp server in time. Otherwise, it will make its way to tracemonkey and mozilla-central in due (review) time.

Thanks in advance.

Comment 23

[Robert Kaiser]

(In reply to comment #22)
> I tried running mochitest-chrome for the seamonkey
> build locally on my linux machine in hopes of creating a reduced test case but
> didn't see any failures, so I assume I was just Doing It Wrong.

Erm, did you run mochitest-chrome or mochitest-browser-chrome? The latter is what's failing badly for us right now, the former is good on tinderbox as well. Note that we expect to have a small amount of real failures when this is fixed, but right now, it's more or less completely busted.

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

 TEST_PATH=suite/common/dataman make -C objdir-sm-debug/ mochitest-browser-chrome passes 239 tests. I'm going to say we're probably good.

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

Fixed in tracemonkey per bug 595691 comment 36.

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

Passing assignment to :sgautherie to mark as fixed, since this fix has landed on mozilla-central.

Comment 27

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1296201988.1296203807.31400.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2011/01/28 00:06:28

No more
"Exception thrown at chrome://mochitests/content/browser/... - SyntaxError: missing ; before statement"
and the like :-)

(Now, we just have to sort out the real failures :-<)

V.Fixed

Comment 28

[Robert Kaiser]

Chris, thanks again for working on this issue, after another fix that was strictly on our side, in one of our tests, we are looking really good on that suite now! :)

Comment 29

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #28)
> we are looking really good on that
> suite now!

Glad to hear. My apologies for the temporary interruption in service! ;-)