typo in noscripService.js and a few surrogate questions

[al_9x]

Saw the following logged:

[NoScript] l.parentNode is not a function while processing JS redirects

Presumably it's about line 3756: "if (!(n = n.parentNode())) return true;"

1. I used "let" in a surrogate and noticed it wasn't working. Would it make sense (and is it possible) to enable lang features (like let) for surrogates?

2. Should surrogates perhaps be wrapped in functions by default? Is it a significant perf win, not to?

3. What's the perf hit of file based surrogates? Any other caveats for them, besides lack of syncing, export, import?

[Giorgio Maone]

Saw the following logged:

[NoScript] l.parentNode is not a function while processing JS redirects

Presumably it's about line 3756: "if (!(n = n.parentNode())) return true;"

1. I used "let" in a surrogate and noticed it wasn't working. Would it make sense (and is it possible) to enable lang features (like let) for surrogates?

Please check latest development build 2.1.9rc3.

2. Should surrogates perhaps be wrapped in functions by default? Is it a significant perf win, not to?

You've got one object less to be created and later garbage collected for each surrogate.
If you've got a lot of surrogates and you don't need to isolate your variables it's a win, IMHO, over the convenience of having the code already wrapped when you actually need it.

3. What's the perf hit of file based surrogates? Any other caveats for them, besides lack of syncing, export, import?

Quite a lot, even though probably still negligible if compared with initial non-cached loads from a remote location: they're loaded each time synchronously from the filesystem and they're never cached in memory.

[al_9x]

Please check latest development build 2.1.9rc3.

first a few general question about JS versioning, because there's a problem, it's not clear how it's supposed to work:

1. What is the default version? Is it latest supported minus certain Mozilla extensions requiring explicit version?
2. Is there a way to say (latest supported including all Mozilla extensions), without specifying a number? That's probably what's desired.
3. What is the latest version, 1.8.5?
4. Is the specified version intended to mean "disable all features from later versions" and "enable all Mozilla extension up to this version?"
5. If the current version is 1.8.5, does setting it to 1.8.1 disable both ECMAScript5 and Mozilla extension features from 1.8.5?

now the problems:

1. surrogates are completely broken:

   Code: Select all

   Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.evalInSandbox]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://noscript/content/ScriptSurrogate.js :: <TOP_LEVEL> :: line 240"  data: no]
   Source File: chrome://noscript/content/ScriptSurrogate.js
   Line: 227
   

2. Why swallow surrogate errors in normal mode? Debug mode turns on serialized execution so makes it easier to see what's failing, but knowing that something is failing is still useful in normal mode. Also, evalInSandbox takes a url for exception logging purposes, perhaps you can shove something informative there.
3. The following is unrelated to versioning, just something I noticed in the process:
   On a test page with every surrogate type and certain syntax/parse errors in @ (e.g. use of let),
   whereas, in debug mode, the @ syntax error breaks only the @ surrogate,
   in normal mode, it breaks also the !@ surrogate, it never executes.
   
   What seems to be happening is that if the js engine fails to parse the entire concatenated blob (which is not just surrogates, but js injections backing NS core functionality), then all of it is lost. If so, at a minimum, NS code should be isolated from surrogates, but even different surrogates interfering with each other is not a good thing. Is it too much of a perf hit to isolate them?
   
   Another (theoretical) possibility is to somehow parse them, detecting syntax errors without execution, excluding the bad ones. Does the engine offer such a service? Obviously you would only want to do this exclusion parse once after modification, but that would not work for file based surrogates, whose modification can't be detected.
   
   One mystery does remain, why does the !@ follow @ in the concatenated blob? They are supposed to execute at different times.

[dhouwn]

Is it latest supported minus certain Mozilla extensions requiring explicit version?

I guess so:

The features that require the use of the new keywords "yield" and "let" require you to specify version 1.7 or higher because existing code might use those keywords as variable or function names. The features that do not introduce new keywords (such as generator expressions) can be used without specifying the JavaScript version.

Is there a way to say (latest supported including all Mozilla extensions), without specifying a number? That's probably what's desired.

Good question, no idea.

What is the latest version, 1.8.5?

I believe it is the last versioned version.

[Giorgio Maone]

Please check latest development build 2.1.9rc3.

first a few general question about JS versioning, because there's a problem, it's not clear how it's supposed to work:
What is the default version? Is it latest supported minus certain Mozilla extensions requiring explicit version?

It's the latest version including all the Mozilla-specific extensions for chrome components and modules, and the latest version minus syntax incompatible with ECMAScript for content (i.e. new/extended host objects work, but new keywords / syntactic sugar like "let", "yield" or shorthanded functions don't).
In other words, if and only if you want to use "let" "yield" & C. in a XUL or HTML page, you need to specify a version equal or higher 1.7 (1.8 for braceless functions).

Is there a way to say (latest supported including all Mozilla extensions), without specifying a number? That's probably what's desired.

None that I know.

What is the latest version, 1.8.5?

Yes it is, but apparently nothing higher than 1.8 seems to be accepted as an explicit version number, probably because that's the latest version with incompatible keywords/syntax.

Is the specified version intended to mean "disable all features from later versions" and "enable all Mozilla extension up to this version?"

Yes, AFAIK. The supposed rationale is preventing older scripts from breaking (e.g. by using "yield" as a function name).

If the current version is 1.8.5, does setting it to 1.8.1 disable both ECMAScript5 and Mozilla extension features from 1.8.5?

There's no Mozilla extension feature that I know about past 1.8.
Regarding ECMAScript 5 features, they all work since there's no syntax incompatibility.

now the problems:

surrogates are completely broken

This is due to using 1.8.1 rather than 1.8, something I already had fixed but was left out by accident from rc3 (I was sure it was there, but it wasn't). Fixed in rc4

Why swallow surrogate errors in normal mode?

Because, since they run in a hostile/unaware environment by definition, some surrogates are expected to fail at runtime sometimes, and you don't want to pollute the console (and have a performance hit too, from this) unless you're debugging.

What seems to be happening is that if the js engine fails to parse the entire concatenated blob (which is not just surrogates, but js injections backing NS core functionality), then all of it is lost.

Yes, it's expected because they're a single parse unit.

If so, at a minimum, NS code should be isolated from surrogates

It is.

but even different surrogates interfering with each other is not a good thing.

You're not expected to keep surrogates with syntax errors in place. And if you're fiddling with your own surrogates, you're supposed to be a geek and able to clean up your mess, aren't you?

Is it too much of a perf hit to isolate them?

Yes it is. It means multiple sandboxes / script DOM elements to be inserted, instead of just one.

Another (theoretical) possibility is to somehow parse them, detecting syntax errors without execution, excluding the bad ones. Does the engine offer such a service?

This is actually a good idea. The browse does offer a parse-only facility in very recent versions (actually I must check whether it's on trunk/betas or still in a separate experimental branch), but I've been doing this by hacking the old good Sandbox in InjectionChecker for years (I just run "new Function(scriptText)", to parse but not execute, and see if it sticks).

Obviously you would only want to do this exclusion parse once after modification, but that would not work for file based surrogates, whose modification can't be detected.

Of course. Putting this in my TODO list.

One mystery does remain, why does the !@ follow @ in the concatenated blob? They are supposed to execute at different times. [/list]

They're in the same blob, but the !@ ones are wrapped inside addEventListener("DOMContentLoaded", function() { ... }, false).

[al_9x]

If the current version is 1.8.5, does setting it to 1.8.1 disable both ECMAScript5 and Mozilla extension features from 1.8.5?

There's no Mozilla extension feature that I know about past 1.8.
Regarding ECMAScript 5 features, they all work since there's no syntax incompatibility.

So just to confirm, nothing from https://developer.mozilla.org/En/JavaSc ... ript/1.8.1 or https://developer.mozilla.org/en/JavaSc ... ript/1.8.5 is disabled when you set it to 1.8?

If so, at a minimum, NS code should be isolated from surrogates

It is.

Not AFAICT. Most of the blob is not surrogates, but code backing built-in NS functionality, and it's lost due to a surrogate error. How is that isolated?

e.g. non surrogate code in the blob:

Code: Select all

try {
	(function () {
		var type = "application/x-shockwave-flash";
		var ver;
		var setAttribute = HTMLObjectElement.prototype.setAttribute;
		HTMLObjectElement.prototype.setAttribute = function (n, v) {
			if (n == "type" && v == type && !this.data) {
				this._pendingType = v;
				this.SetVariable = function () {};
				this.GetVariable = function (n) {
					if (n !== "$version") {
						return undefined;
					}
					if (!ver) {
						ver = navigator.plugins['Shockwave Flash'].description.match(/(\d+)\.(\d+)(?:\s*r(\d+))?/);
						ver.shift();
						ver.push("99");
						ver = "WIN " + ver.join(",");
					}
					return ver;
				};
			}
			setAttribute.call(this, n, v);
			if (n === "data" && "_pendingType" in this && this._pendingType === type) {
				setAttribute.call(this, "type", type);
				this._pendingType = null;
			}
		};
	})()
} catch (e) {}

One mystery does remain, why does the !@ follow @ in the concatenated blob? They are supposed to execute at different times.

They're in the same blob, but the !@ ones are wrapped inside addEventListener("DOMContentLoaded", function() { ... }, false).

I was actually expecting to see DOMContentLoaded, but didn't. I think I found the problem. My test surrogates are file based, and if the code that wraps !@ in a DOMContentLoaded is on line 145, then it does not run for files based ones. Also, in the interest of getting it to run ahead of any possible content listener, what about attaching it to window in capturing mode?

[Giorgio Maone]

So just to confirm, nothing from https://developer.mozilla.org/En/JavaSc ... ript/1.8.1 or https://developer.mozilla.org/en/JavaSc ... ript/1.8.5 is disabled when you set it to 1.8?

Nothing, in fact.

Most of the blob is not surrogates, but code backing built-in NS functionality, and it's lost due to a surrogate error.

You're right on that one. One more reason for early syntax check (I'm likely to put it in 2.2rc1).

My test surrogates are file based, and if the code that wraps !@ in a DOMContentLoaded is on line 145, then it does not run for files based ones. Also, in the interest of getting it to run ahead of any possible content listener, what about attaching it to window in capturing mode?

Fix and change will go in 2.2rc1 as well.

[al_9x]

2. Should surrogates perhaps be wrapped in functions by default? Is it a significant perf win, not to?

You've got one object less to be created and later garbage collected for each surrogate.
If you've got a lot of surrogates and you don't need to isolate your variables it's a win, IMHO, over the convenience of having the code already wrapped when you actually need it.

The reason I thought they should be normalized to all be in functions is because some are already in their own event handlers while others are not. That should be documented, if it stays this way. The overhead, extra object per surrogate, does not seem significant, and a justifiable price for a simpler, more consistent model, and terser code. I am on a slow system and would be the first to argue against any perf degradation, but it seems this should not be perceptible. Am I mistaken? Are you firm on this?

[al_9x]

Since surrogates are wrapped in try blocks, lets are already isolated, however in debug mode they are not. Can you wrap top level surrogates in { } blocks in debug mode, for semantic consistency?

[Giorgio Maone]

Since surrogates are wrapped in try blocks, lets are already isolated, however in debug mode they are not. Can you wrap top level surrogates in { } blocks in debug mode, for semantic equivalence?

OK

[al_9x]

My test surrogates are file based, and if the code that wraps !@ in a DOMContentLoaded is on line 145, then it does not run for files based ones. Also, in the interest of getting it to run ahead of any possible content listener, what about attaching it to window in capturing mode?

Fix and change will go in 2.2rc1 as well.

would this do it:

Code: Select all

diff -U 8 -r noscript-2.2.2rc5/chrome/content/noscript/ScriptSurrogate.js noscript-2.2.2rc5.5/chrome/content/noscript/ScriptSurrogate.js
--- chrome/content/noscript/ScriptSurrogate.js	2011-12-01 15:38:52.000000000 -0500
+++ chrome/content/noscript/ScriptSurrogate.js	2011-12-01 17:18:16.653717700 -0500
@@ -147,20 +147,20 @@
             code = IO.readFile(IOS.newURI(this._resolveFile(mapping.replacement), null, null)
                                .QueryInterface(Ci.nsIFileURL).file);
           } catch(e) {
             ns.dump("Error loading " + mapping.replacement + ": " + e);
             continue;
           }
         } else {
           code = mapping.replacement;
-          if (!noScript && mapping.noScript)
-            code = 'document.addEventListener("DOMContentLoaded", function(event) {' +
-                    code + '}, false)';
         }
+        if (!noScript && mapping.noScript)
+          code = 'addEventListener("DOMContentLoaded", function(event) {' +
+                  code + '}, true)';
         if (!scripts) scripts = [code];
         else scripts.push(code);
       }
     }
     return scripts;
   },
   
   _afterHandler: function(ev) {

[Giorgio Maone]

Mostly

[al_9x]

Mostly

Please clarify, why document? Isn't attaching to the window in capturing mode, the earliest possible invocation?

AFAICT, changing to capturing mode when registering on the target of the event (document in this case) doesn't do anything.

[Giorgio Maone]

Mostly

Please clarify, why document? Isn't attaching to the window in capturing mode, the earliest possible invocation?

Yes it is, just another typo.
Fixed in 2.2.3rc2, thanks.

[al_9x]

Just want to make sure I am not missing something. Is "window." that you added, necessary? It seems like it shouldn't be since it's evaled in the correct context with "window.eval"