Closed Bug 596580 Opened 14 years ago Closed 14 years ago

javascript let statement doesn't work when loaded using mozIJSSubScriptLoader

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: daniel, Assigned: cdleary)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(4 files, 2 obsolete files)

Test case
339 bytes, application/xhtml+xml
Details
test xpi
1.82 KB, application/x-xpinstall
Details
Testcase to be placed in a chrome://test/content/ dir
820 bytes, application/zip
Details
Fix for subscript loader.
11.26 KB, patch
brendan
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100915

When loading another script using mozIJSSubScriptLoader the let statement doesn't work anymore in the loaded script.

Works: 20100914 id=5588c9796f0b
Fails: 20100915 id=0caec4ddff74

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5588c9796f0b&tochange=0caec4ddff74

Reproducible: Always
Keywords: regression
Tested in latest XULRunner trunk
Almost certainly related to http://hg.mozilla.org/mozilla-central/rev/33bf77bcf1a0 -- do you have a test case?
Assignee: general → cdleary
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Attached file Test case — 
I don't have a test case for this problem, but as my bug has been marked a duplicate of this, failing test case attached.
Attached file test xpi — 
I can not reproduce the problem of "let".

[STR]
1. Start Minefield with new profile and  -jsconsole
2. Install test xpi and restart to complete instalattion.
3. Open Error Console
4. Alt > Tool > alert popup bug596580
and
5. Alt > Help > About Minefield

[Actual]
 No Error, and result is as expected.
 An alert box popup. and it contains as follows:

   bug596580
The testcase tries to load a file that doesn't exist. After that a second, existing file will be loaded using a setTimeout call. Using this combination I was able to reproduce the behaviour in my simplified testcase.
The testcase needs to be placed in a chrome://test/content/ directory.

A full testcase embedded in a XULRunner from 2010.09.16 can be found at:
http://birgin.de/test/bugzilla/596580_full_testcase.zip
Check the two files in the /chrome/content/ directory
Blocks: 596915
Chris, do you know what's going on here? Might this be a beta-blocker?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #7)
> Chris, do you know what's going on here? Might this be a beta-blocker?

Still looking into it, figuring out what version it was using historically (since it uses EvaluateScriptForPrincipals without explicitly setting a version). Not sure what the typical qualifications are for beta blockage.
JSVERSION_DEFAULT is web-compatible forward-evolving intersection semantics. But 'let' and CDATA marked sections from E4X require more opt-in:

xpconnect/loader/mozJSComponentLoader.cpp:    JS_SetVersion(mContext, JSVERSION_LATEST);

Did this used to affect the subscript loader, and now does not?

/be
Attached patch Fix for subscript loader. (obsolete) — — Splinter Review 
I believe this fixes the issue. There must be something setting a version override higher up in the stack that we need to forcibly ignore (as is done with this patch). I'll figure out what that is before putting the fix up for review.
(In reply to comment #5)
> Created attachment 475810 [details]
> test xpi
> 
> I can not reproduce the problem of "let".
> 
> [STR]
> 1. Start Minefield with new profile and  -jsconsole
> 2. Install test xpi and restart to complete instalattion.
> 3. Open Error Console
> 4. Alt > Tool > alert popup bug596580
> and
> 5. Alt > Help > About Minefield
> 
> [Actual]
>  No Error, and result is as expected.
>  An alert box popup. and it contains as follows:
> 
>    bug596580

This extension seems to work fine on my mozilla-central build now... (Creates the "actual" results for both menu items.) Not sure what changed because I was able to repro the other day.
Status: NEW → ASSIGNED
Chris, should it fix only bug 596502?
I can still reproduce the let problem with the testcase from attachment 475872 [details] and a XULRunner nightly from 20100921.
(In reply to comment #12)
> Chris, should it fix only bug 596502?

Will figure out the cause of this tomorrow, finally figured out how to build a debug xulrunner today. :-)
Comment on attachment 476338 [details] [diff] [review]
Fix for subscript loader.

This indeed fixes the xulrunner testcase. (Indicates I should move the rest of the browser uses to the versioned API sooner rather than later.)
Attachment #476338 - Flags: review?(sayrer)
Attachment #476338 - Attachment description: WIP: Fix for subscript loader. → Fix for subscript loader.
Comment on attachment 476338 [details] [diff] [review]
Fix for subscript loader.

line breaking on the long parameter list is kind of unfortunate. I like the way you have it in jsapi.h. Over to Brendan for jsapi change.
Attachment #476338 - Flags: review?(sayrer)
Attachment #476338 - Flags: review?(brendan)
Attachment #476338 - Flags: review+
Comment on attachment 476338 [details] [diff] [review]
Fix for subscript loader.

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -4796,16 +4796,26 @@ JS_EvaluateScriptForPrincipals(JSContext
>         return JS_FALSE;
>     JSBool ok = JS_EvaluateUCScriptForPrincipals(cx, obj, principals, chars, length,
>                                                  filename, lineno, rval);
>     cx->free(chars);
>     return ok;
> }
> 
> JS_PUBLIC_API(JSBool)
>+JS_EvaluateScriptForPrincipalsVersion(JSContext *cx, JSObject *obj, JSPrincipals *principals,
>+                                      const char *bytes, uintN nbytes,
>+                                      const char *filename, uintN lineno, jsval *rval, JSVersion version)

Need a preposition before "Version", to go with "ForPrincipals" and not seem to run that together so the call is for a version of the principals. "With" or "Using" seem ok.

>-    ok = JS_EvaluateScriptForPrincipals (cx, target_obj, jsPrincipals,
>-                                         buf, len, uriStr.get(), 1, rval);        
>+    ok = JS_EvaluateScriptForPrincipalsVersion(cx, target_obj, jsPrincipals,
>+                                               buf, len, uriStr.get(), 1, rval,
>+                                               JSVersion(script->getVersion() &
>+                                                         ~js::VersionFlags::ANONFUNFIX));

Nit: non-&&/|| ops go at start of overflow line in prevailing style, usually.

Non-nit: I see how this fixes things, but why do you clear ANONFUNFIX?

I'm concerned the move from cx->version to script-compiled-in version has left other such bugs, but I guess they'll come out in the wash, if they exist.

r=me with these points addressed.

/be
Attachment #476338 - Flags: review?(brendan) → review+
(In reply to comment #16)
> Need a preposition before "Version", to go with "ForPrincipals" and not seem to
> run that together so the call is for a version of the principals. "With" or
> "Using" seem ok.

Hmm, I should probably make a file-up for this, since the other version-explicit APIs run together. WithVersion just seemed so much longer when it was prefixed with JS_EvaluateUCScriptForPrincipals. :-)

> Non-nit: I see how this fixes things, but why do you clear ANONFUNFIX?

The only version flag permitted by the new API is js::VersionFlags::HAS_XML. It's that way because nsJSVersionSetter only propagated the JSVERSION_HAS_XML flag into the options, and that's the thing this Version'd API was effectively eliminating. It would be more forward looking (but slightly uglier) to do ``JSVersion((script->getVersion() & js::VersionFlags::MASK) | (script->getVersion() & js::VersionFlags::HAS_XML))``. Will make it that way for clarity.

> I'm concerned the move from cx->version to script-compiled-in version has left
> other such bugs, but I guess they'll come out in the wash, if they exist.

Me too, will try to get cracking on bug 595691 to make version-setting more consistent throughout the browser and move existing calls to their WithVersion alternatives.
(In reply to comment #17)
> (In reply to comment #16)
> > Need a preposition before "Version", to go with "ForPrincipals" and not seem to
> > run that together so the call is for a version of the principals. "With" or
> > "Using" seem ok.
> 
> Hmm, I should probably make a file-up for this,

Please do file a followup -- trivial rs=me.

> since the other version-explicit APIs run together. WithVersion just seemed so
> much longer when it was prefixed with JS_EvaluateUCScriptForPrincipals. :-)

That's just the way the API cookie crumbles. It's better to be long when there are more gotchas than to be too short -- or grammatically incorrect!

> > Non-nit: I see how this fixes things, but why do you clear ANONFUNFIX?
> 
> The only version flag permitted by the new API is js::VersionFlags::HAS_XML.
> It's that way because nsJSVersionSetter only propagated the JSVERSION_HAS_XML
> flag into the options, and that's the thing this Version'd API was effectively
> eliminating. It would be more forward looking (but slightly uglier) to do
> ``JSVersion((script->getVersion() & js::VersionFlags::MASK) |
> (script->getVersion() & js::VersionFlags::HAS_XML))``. Will make it that way
> for clarity.

Both XML and ANONFUNFIX were sync'ed from options to version. I don't see a good reason to favor one over the other. This looks like bug-bait to me.

> > I'm concerned the move from cx->version to script-compiled-in version has left
> > other such bugs, but I guess they'll come out in the wash, if they exist.
> 
> Me too, will try to get cracking on bug 595691 to make version-setting more
> consistent throughout the browser and move existing calls to their WithVersion
> alternatives.

Yes, EIBTI. Just need to be E.

/be
Blocks: 600657
Blocks: 601458
This got backed out because it was burning xpcshell tests:
http://hg.mozilla.org/tracemonkey/rev/12af760b7acb

Need to investigate and repush.
Whiteboard: fixed-in-tracemonkey
(In reply to comment #20)
> Need to investigate and repush.

Couldn't repro xpcshell-tests failure on my box, pushed to try to see what happens there: http://hg.mozilla.org/try/rev/56eb2a29b17d
Blocks: 597811
Version: unspecified → Trunk
Attached patch Fix for subscript loader. (obsolete) — — Splinter Review 
Okay, the one test that was failing on XPCShell tests was xpconnect/tests/unit/bug596580_versioned.js

It was throwing an "Exception" instead of an error (test writer was probably drinking too much Python ;-) and the stack dumper was choking on an unexpected "-e" style stack frame signature, which is why we saw it as a head.js exception.

Once that was fixed, it pointed out the larger issue that XPCShell tests expect the subscript loader to obey a forced version number. I updated the version-finder in the subscript loader to use |cx->findVersion|.
Attachment #476338 - Attachment is obsolete: true
Comment on attachment 483591 [details] [diff] [review]
Fix for subscript loader.

I guess I forgot to flag a r? ...
Attachment #483591 - Flags: review?(sayrer)
Comment on attachment 483591 [details] [diff] [review]
Fix for subscript loader.


>+        /* The API doesn't like getting anything other than the XML flag. */

This comment is cryptic. Can you make it better?
Comment on attachment 483591 [details] [diff] [review]
Fix for subscript loader.

>+        JSVersion version = cx->findVersion();
>+        /* The API doesn't like getting anything other than the XML flag. */

Blank line before comment taking one or more lines unless prior non-space char is {.

>+        JSVersion mungedVersion = js::VersionNumber(version);
>+        js::VersionSetXML(&mungedVersion, js::VersionHasXML(version));

Nit: is use namespace js appropriate here to relieve the js:: eyestrain?

Non-nit: why shouldn't this be done by cx->findVersion?

In the old code, cx->version was sticky and made the compiler bake in the right bits, along with XML and ANONFUNFIX options encoded as version flags, right?

To transform that to the new code, we need both cx->findVersion() and JS_{Compile,Evaluate}*Version API usage. But I don't see why any more open-coded logic is required.

In fact, looking at jscntxt.h (I hadn't read this code before):

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 */
. . .
static inline JSVersion
VersionNumber(JSVersion version)
{
    return JSVersion(uint32(version) & VersionFlags::MASK);
}

static inline bool
VersionHasXML(JSVersion version)
{
    return !!(version & VersionFlags::HAS_XML);
}
. . .
static inline void
VersionSetXML(JSVersion *version, bool enable)
{
    if (enable)
        *version = JSVersion(uint32(*version) | VersionFlags::HAS_XML);
    else
        *version = JSVersion(uint32(*version) & ~VersionFlags::HAS_XML);
}

So all you are doing with these lines:

        JSVersion mungedVersion = js::VersionNumber(version);
        js::VersionSetXML(&mungedVersion, js::VersionHasXML(version));

is (expanding by hand and simplified to pseudo-code):

        JSVersion mungedVersion = version & 0x0FFF;
        if (version & HAS_XML)
            mungedVersion |= HAS_XML;
        else
            mungedVersion &= ~HAS_XML;

which seems no better than

        JSVersion mungedVersion = version;

I still don't understand comment 17 on why ANONFUNFIX should not be kept too. The old way, sticky cx->version, didn't treat it separately from XML. We want ANONFUNFIX to stick, it's the ECMA-262 compatible way and I just enabled it in the js shell (but I forgot xpcshell, d'oh!). It seems to me your patch loses ANONFUNFIX for subscripts.

Oh wait! VersionHasAnonFunFix is unused except to sync the bits between options and version. The parser tests cx->options & JSOPTION_ANONFUNFIX directly.

Ok, that means you should get rid of VersionFlags::ANONFUNFIX, VersionsHasAnonFunFix, etc. Please do in a revision to this patch, it's just confusing deadwood and it should not survive this patch.

>+        if (charset)
>         {
>+            nsString script;
>+            rv = nsScriptLoader::ConvertToUTF16(
>+                    nsnull, reinterpret_cast<PRUint8*>(buf.get()), len,
>+                    nsDependentString(reinterpret_cast<PRUnichar*>(charset)), nsnull, script);
>+
>+            if (NS_FAILED(rv))
>+            {
>+                errmsg = JS_NewStringCopyZ (cx, LOAD_ERROR_BADCHARSET);
>+                goto return_exception;
>+            }
>+            ok = JS_EvaluateUCScriptForPrincipalsVersion(cx, target_obj, jsPrincipals,
>+                                                         reinterpret_cast<const jschar*>(script.get()),
>+                                                         script.Length(), uriStr.get(), 1, rval,
>+                                                         mungedVersion);
>         }
>+        else
>+        {
>+            ok = JS_EvaluateScriptForPrincipalsVersion(cx, target_obj, jsPrincipals,
>+                                                       buf, len, uriStr.get(), 1, rval,
>+                                                       mungedVersion);
>+        }

Nit: still think we want something, perhaps "And", before "Version".

/be
Attachment #483591 - Flags: review-
Getting rid of VersionFlags::ANONFUNFIX means no need for mungedVersion, so (I hope) no need for VersionSetXML. If you can get rid of all uses and just pass the version found by cx->findVersion() along to the new JS_*Version API call, then please simplify further by removing even more Version* deadwood in jscntxt.h.

/be
Nice catch... I suppose nobody actually relied on the sticky behavior of ANONFUNFIX!
Attachment #483591 - Attachment is obsolete: true
Attachment #486406 - Flags: review?(brendan)
Attachment #483591 - Flags: review?(sayrer)
Attachment #486406 - Flags: review?(brendan) → review+
This bug has the potential to badly break Firefox as seen in Bug 601458. I suspect it was involved in my losing all my tabs and tab groups upon updating from b6 to b7 (more details, just ask). Since this bug lists "fixed-in-tracemonkey", would it be possible to get the fix into b8?
How can I test the fix? The current nightlies still do not work.
(In reply to comment #30)
> How can I test the fix? The current nightlies still do not work.

The fix hasn't been merged into mozilla-central yet - you should be able to grab a build from the tree labeled "tracemonkey".
I've downloaded the 20101111 release from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-tracemonkey/

Using it, I can still reproduce the error in the given testcase and in my application.

Build details:
Built from http://hg.mozilla.org/tracemonkey/rev/3d63107fc788

Configure arguments:
--enable-application=browser --enable-update-channel=nightly-tracemonkey --enable-update-packaging --enable-jemalloc

Do I have a wrong build?

BTW: Are there XULRunner tracemonkey builds anywhere?
I would like to help and test. This bug is blocking our current development.  We cannot use newer builds and check out new features or find other regressions/bugs.

If there is a fixed XULRunner/Firefox version, please provide a download URL.
(In reply to comment #34)
> We cannot use newer builds and check out new features or find other
> regressions/bugs.

Okay, I'm splitting your testcase into its own bug and resolving this one -- it clearly wasn't the same issue, though it is eerily similar: bug 613383.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Hmmm... I don't really understand that. I reported the issue and created a testcase which is still not working for me. I used the Firefox tracemonkey build to test. So it's not just a XULRunner issue.

But actually I don't need to understand the real issue as long as you do :-)
Thanks for the new report.
(In reply to comment #36)
> I reported the issue and created a
> testcase which is still not working for me. I used the Firefox tracemonkey
> build to test. So it's not just a XULRunner issue.

Can you provide the Firefox-based testcase in bug 613383 please? It must be sufficiently different from http://hg.mozilla.org/mozilla-central/file/09ebe83273b7/js/src/xpconnect/tests/unit/test_bug596580.js as to warrant its own bug.

Thanks for following up and clarifying!
The testcase is the same as in comment 6. I just used the firefox tracemonkey build to test it. And yes, it is different from http://hg.mozilla.org/mozilla-central/file/09ebe83273b7/js/src/xpconnect/tests/unit/test_bug596580.js

I will recheck the latest builds on monday and provide feedback in bug 613383. Maybe I've done something wrong.

I assume that the patch will be in the latest nightlies for both XULRunner and Firefox, right?
I can verify that this patch has been fixed the problems we had with Mozmill. I'm using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre

I will leave it as resolved for now, until we get a response from Daniel on Monday.
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b8
I can still reproduce the error. I suggest keeping this one fixed and continue in bug 613383 for the special case I've found.
Sounds good to me. Marking as verified fixed given my comment from above.
Status: RESOLVED → VERIFIED
No longer blocks: 619713
Blocks: 619713
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: