Closed Bug 609942 Opened 9 years ago Closed 9 years ago

js1_5/extensions/regress-376052.js fails (all jit flags)

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: luke, Assigned: cdleary)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

This fails on a debug shell, Ubuntu x86.

/moz/safe/js/src/tests $ python jstests.py ../objdbg/js -o --args='' js1_5/extensions/regress-376052.js
    rc = 0, run time = 0.020800
BUGNUMBER: 376052
STATUS: javascript.options.anonfunfix to allow function (){} expressions
 PASSED! javascript.options.anonfunfix to allow function (){} expressions: 1
 PASSED! javascript.options.anonfunfix to allow function (){} expressions: 2
 FAILED! [reported from test()] javascript.options.anonfunfix to allow function (){} expressions: 3 : Expected value 'SyntaxError: syntax error', Actual value 'No Error' 
REGRESSIONS
    js1_5/extensions/regress-376052.js
FAIL
blocking2.0: --- → ?
Regressing changeset:

changeset:   56632:09ebe83273b7
user:        Chris Leary <cdleary@mozilla.com>
date:        Wed Nov 03 12:09:24 2010 -0700
summary:     Bug 596580: Fix mozJSSubScriptLoader's version finding. (r=brendan)
Isn't the problem that the test assumes shell anonfunfix option is off when it is in fact now on?

/be
(In reply to comment #2)
> Isn't the problem that the test assumes shell anonfunfix option is off when it
> is in fact now on?

It looks like it actually relates to the eval cache. The test basically tries this twice:

   eval('function () {1;}');

(It also does a version with parens, but that's not important.) The first time, it wants no error, and anonfunfix is off, so we do get no error. It toggles the option and does it the second time, expecting an error, but it doesn't get one.

Debugging shows that we don't even run Parser::functionStmt the second time through, so there is no possibility to generate an error. I figure that's the eval cache.

But if so, why would http://hg.mozilla.org/tracemonkey/rev/09ebe83273b7 cause a regression here? That patch only does stuff with versions and options.
Ah, brilliant. This is why we can't remove propagation of cx->options & ANONFUNFIX to script->version, like we thought in bug 596580 comment 27.

I'll put this back the way it was and add a comment on why JSVERSION_ANONFUNFIX can't be removed. (Note that there's still no need for mungedVersion in that bug, like Brendan was saying originally -- the ANONFUNFIX version setting from the script up the stack should be propagated downwards.)
Assignee: general → cdleary
Status: NEW → ASSIGNED
Ook. Eval cache. Good find, dmandelin.

/be
This passes regression -- only one weirdness of note, which was also noted in the previous bugs:

The existing clients of the JS_*Version APIs do not expect the JSVersion ANONFUNFIX bit of the version argument to be relevant (we fail tests if we consider it relevant). Bug 595691 will smooth this out, such that the client can specify {don't care, enable, disable} for both the JSVersion ANONFUNFIX and HAS_XML. The current JS_*Version interface conflates ~HAS_XML and "don't care about XML".

I'm going to take a short break from PICs to finish that bug off tomorrow because we keep bumping into this unacceptable stop-gap.
Attachment #489061 - Attachment is obsolete: true
Attachment #489424 - Flags: review?(brendan)
Comment on attachment 489424 [details] [diff] [review]
Add anonfunfix back in.

>         cx->options = VersionHasXML(newVersion)
>                       ? (cx->options | JSOPTION_XML)
>                       : (cx->options & ~JSOPTION_XML);
>+        /* 
>+         * Note: ANONFUNFIX is ignored for backwards compatibility, must be set via JS_SetOptions.
>+         */
..1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

Nits: blank line before comment taking one or more lines unless preceded by {, and wrap the comment before 80.

>+/*
>+ * Flags accompany script version data so that a) dynamically created scripts
>+ * can inherit their caller's parse properties and b) scripts can be

s/parse/compile-time/ (bonus: less ragged-rightness).

>+ * appropriately compared in the eval cache across global option changes. An
>+ * example of the latter is enabling the top-level-anonymous-function-is-error
>+ * option: subsequent evals of the same, previously-valid script text may have
>+ * become invalid.
>+ */
> namespace VersionFlags {
>-static const uint32 MASK =        0x0FFF; /* see JSVersion in jspubtd.h */
>-static const uint32 HAS_XML =     0x1000; /* flag induced by XML option */
>+static const uint32 MASK =          0x0FFF; /* see JSVersion in jspubtd.h */
>+static const uint32 HAS_XML =       0x1000; /* flag induced by XML option */
>+static const uint32 ANONFUNFIX =    0x2000; /* see jsapi.h comment on JSOPTION_ANONFUNFIX */

Nit: reduce raggedness by aligning the = to start in the same column.

r=me with these. Nice fixage.

/be
Attachment #489424 - Flags: review?(brendan) → review+
http://hg.mozilla.org/mozilla-central/rev/49a90627ab39
Status: ASSIGNED → RESOLVED
blocking2.0: ? → betaN+
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.