595917 - JM: D-Link Router JavaScript Navigation Links broken since JM Merge

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[d.a.]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre

The JM merge has caused the following error when visiting the router page, breaking all links on the page as well as all submit-buttons as these links are handled via JavaScript.

Error:
Error: uncaught exception: [Exception... "Component returned failure code: 0x805e0006 [nsIXMLHttpRequest.open]"  nsresult: "0x805e0006 (<unknown>)"  location: "JS frame :: http://192.168.1.1/ajax.js :: anonymous :: line 113"  data: no]

Code block (line 113 is req.open):
if (!window.DOMParser) {
	var DOMParser = function() { };

	DOMParser.prototype.parseFromString = function(xmlstr, content_type) {
		if (window.ActiveXObject && MSXML_DOM_ID) {
			var doc = new ActiveXObject(MSXML_DOM_ID);
			doc.async = false;
			doc.loadXML(xmlstr);
			return doc;
		}
		if (window.XMLHttpRequest) {
			var req = new XMLHttpRequest();
			if (!content_type) {
				content_type = "application/xml";
			}
			req.open("GET", "data:" + content_type + ";charset=utf-8," + encodeURIComponent(xmlstr), false);
			if (req.overrideMimeType) {
				req.overrideMimeType(content_type);
			}
			req.send(null);
			return req.responseXML;
		}
		return null;
	};
}


Reproducible: Always

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

For Reference:
Does setting javascript.options.methodjit.content;false and/or javascript.options.tracejit.content;false help?

Comment 2

[d.a.]

Disabling either of those has no effect, nor does starting the browser in safe-mode.

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

Hmm, then it would be helpful providing a Regression Range using the Tracemonkey Repo Branch using Builds from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ on the "-tracemonkey/" flagged Dirs.

You might start with Builds before and after "Tue Aug 31 13:54:35 2010" when JM landed on TM.

Comment 5

[Gary [:streetwolf52]]

Regression Found:

changeset - 53643:f1bd314e64ac   Does NOT work.

16eac4b8b8e0  Works


Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100911 Firefox/4.0b6pre - Build ID: 20100913134903

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to comment #5)
It's already known that this is a JM Merge Regression - but thanks anyways.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Does this need to block the next beta?

Comment 8

[Gary [:streetwolf52]]

I vote to make it a blocker for the next beta.  Whose to know this doesn't affect just Dlink router pages.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100915 Firefox/4.0b7pre - Build ID: 20100915102204

Comment 9

[Brendan Eich [:brendan]]

Is it possible to attach the complete relevant content, HTML and JS and CSS?

/be

Comment 10

[Gary [:streetwolf52]]

(In reply to comment #9)
> Is it possible to attach the complete relevant content, HTML and JS and CSS?
> 
> /be

Are you asking this of me Brendan?  If so, do you mean a copy of the source for a particular page from my router using the 'View Page Source' option?

Comment 11

[Gary [:streetwolf52]]

Attachment: A Dlink router page for analysis.

Comment 12

[Gary [:streetwolf52]]

Attachment: Matching Router page.

Comment 13

[Gary [:streetwolf52]]

Hate to be a nag but is this actively being looked at to resolve it? I just hate having to use IE to access my router.

Comment 14

[Brendan Eich [:brendan]]

A bit backed up, speaking for myself -- if anyone has time to reduce the files Gary attached to a testcase, that would be very helpful.

/be

Comment 15

[Gary [:streetwolf52]]

Attachment: Error Console Log

With my router page open I get the error console messages I have attached.  Maybe it will help with fixing the problem.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100918 Firefox/4.0b7pre - Build ID: 20100918074133

Comment 16

[Jan de Mooij [:jandem]]

Gary, can you also attach the files ../ajax.js, ../utils.js, ../navigation.js? You can just click View Page Source, search for these files and click the links to open them. The error is likely caused by something in one of these files.

Comment 17

[d.a.]

Attachment: Testcase

A testcase, gives the same error as a router page.

No error shows up in a pre-JM build.

Comment 18

[d.a.]

BTW, should the testcase be text/plain or text/html?

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

When I load that (trunk nightly based off http://hg.mozilla.org/mozilla-central/rev/fc8d8dfee20f) I don't get an error.  Tried via HTTP (http://people.mozilla.org/~shaver/router-error-test.html) and off a local file.

Comment 20

[d.a.]

I still get the error (comment 19 link):

Error: uncaught exception: [Exception... "Component returned failure code: 0x805e0006 [nsIXMLHttpRequest.open]"  nsresult: "0x805e0006 (<unknown>)"  location: "JS frame :: http://people.mozilla.org/~shaver/router-error-test.html :: test :: line 10"  data: no]

Comment 21

[Jan de Mooij [:jandem]]

I don't get an error either on that link or locally.

Can you try with a clean profile, extensions disabled, cleared cache, etc?

Comment 22

[d.a.]

Clean profile: no error.
My profile, NoScript disabled: no error.

Seems like this is caused by the NoScript extension. Will report it to the developer.

Comment 23

[d.a.]

The test can be simplified even further, to this:

var req = new XMLHttpRequest();
req.open("GET", "data:,Hello%2C%20World!", false);

Comment 24

[d.a.]

I went to the D-Link Router with the clean profile again and this time I got the error message shown in the png image of the error console. Error:

Error: uncaught exception: [Exception... "Access to restricted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "http://192.168.1.1/ajax.js Line: 117"]

Line 117 is "req.send(null)".

I'm attaching the 3 js files.

Comment 25

[d.a.]

Attachment: ajax.js

Comment 26

[d.a.]

Attachment: navigation.js

Comment 27

[d.a.]

Attachment: utils.js

Comment 28

[d.a.]

Attachment: Modified testcase

This testcase will produce the same error as the d-link router page, even on a clean profile.

Code is this:
var req = new XMLHttpRequest();			
req.open("GET", "data:,Hello%2C%20World!", false);
req.send(null);

Comment 29

[Christian Ascheberg]

Attachment: testcase, special to D-LINK router page

I also have a problem with my D-LINK router, and it might be the same as this one.

For a hopefully better understanding, this testcase contains more of the original D-LINK page. 

What I have found out:
The whole router page has an overlay div "loader_container" which prevents clicking any of the content.
That "loader_container" is not set to "display=none" in FF trunk.
Now see my testcase for details.
In FF 3.6.9 "if (!window.DOMParser)" is always false.
But in FF trunk its true, so then there seems to be the problem with "req.send(null);"
If that "if"-statement was not there, FF 3.6.9 would have the same problem there.

(So maybe someone can tell what this bug is really about?)

Comment 30

[d.a.]

Attachment: Greasemonkey script workaround

Simple Greasemonkey script workaround. With this installed the router works as expected (it hides "loder_container").


If NoScript is installed xmlhttprequest.open fails if a data URI is used as the URL.

If gets past the xmlhttprequest.open and gets to xmlhttprequest.send, it will fail if the URL is a data URI.

Was there any work done with the JS-engine that relates to data URIs?

Comment 31

[Gary [:streetwolf52]]

(In reply to comment #30)
> Created attachment 476563 [details]
> Greasemonkey script workaround
> 
> Simple Greasemonkey script workaround. With this installed the router works as
> expected (it hides "loder_container").
> 
> 
> If NoScript is installed xmlhttprequest.open fails if a data URI is used as the
> URL.
> 
> If gets past the xmlhttprequest.open and gets to xmlhttprequest.send, it will
> fail if the URL is a data URI.
> 
> Was there any work done with the JS-engine that relates to data URIs?

This workaround works fine.  Looking forward to a 'real' fix.

Comment 32

[Boris Zbarsky [:bzbarsky]]

The relevant script here is:

  if (!window.DOMParser) {
    var DOMParser = function() { };
    /* do some stuff here involving an XHR of a data: URI
  }

in ajax.js at toplevel scope.  On trunk and Firefox 3.6, the |var| is hoisted to toplevel (in that if I change "DOMParser" to "foo" throughout then |"foo" in window| right before the if is true), but in Firefox 3.6 it looks like the hoisting doesn't clear the existing value of window.DOMParser, whereas on trunk the hoisting makes it undefined.  I _think_ the latter behavior is what the spec calls for and that we fixed a bug, but it would be good to confirm.  If so, then this DLink code is just totally buggy, of course.

In any case, if the new JS side behavior is correct then the upshot is that the page used to use our built-in DOMParser, but now it uses its own object defined for parsing XML, because they detect us as not having a DOMParser due to the issue above.  Their own object sticks the XML into an XMLHttpRequest GET of a data: URI, which we don't allow because it's not a same-origin request.  Note bug 270748 and that the spec here has changed several times; I'm not sure what it says now.  Jonas?

Comment 33

[Boris Zbarsky [:bzbarsky]]

And thanks to everyone for posting those scripts and reduced testcases!  That was a huge help in figuring out what's going on.

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #32)
> The relevant script here is:
> 
>   if (!window.DOMParser) {
>     var DOMParser = function() { };
>     /* do some stuff here involving an XHR of a data: URI
>   }
> 
> in ajax.js at toplevel scope.  On trunk and Firefox 3.6, the |var| is hoisted
> to toplevel (in that if I change "DOMParser" to "foo" throughout then |"foo" in
> window| right before the if is true), but in Firefox 3.6 it looks like the
> hoisting doesn't clear the existing value of window.DOMParser, whereas on trunk
> the hoisting makes it undefined.  I _think_ the latter behavior is what the
> spec calls for

No. A var never replaces an existing property of the relevant scope object (or binding in an environment record), and hoisting does not assign undefined, it only creates if there is no such property (binding) with initial value undefined. This is a JM bug, and it probably affects a number of sites and JS content instances.

/be

Comment 35

[Brendan Eich [:brendan]]

This should probably block b7.

/be

Comment 36

[Boris Zbarsky [:bzbarsky]]

Brendan, thanks for the spec-check.  :)  Blocking b7 it is.

Comment 37

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thank you everyone for helping reduce this. The problem is in the global variable optimizations introduced in JM. Here's a shell test case that shows the problem:

> if (!this.parseInt) {
>     var parseInt = function () { return 5; }
> }
> assertEq(parseInt(10), 10);

The parser checks whether |parseInt| is already a property on the global object. In this case, |parseInt| is not, because it's added via a resolver. So the compiler defines it ahead of time and initializes it to |undefined|.

This means the resolver never gets called and parseInt is never initialized to the right value.

Comment 38

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

Not pictured in that patch: A big comment explaining that addGlobalUse() initially maps into globalScope->defs. After emitting the outermost script, we traverse the script tree and fix up global indexes to their correct slots.

(Adding such a comment now.)

Comment 41

[Brendan Eich [:brendan]]

Looks good at a quick read-thru, if you want to attach the patch with the big comment I'll read and mark in approx. 2 hours.

/be

Comment 42

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: commented fix

Comment 43

[Brendan Eich [:brendan]]

Comment on attachment 477006 [details] [diff] [review]
commented fix

>  * If the code being compiled is global code, the cloned regexp are stored in
>- * fp->vars slot after cg->ngvars and to protect regexp slots from GC we set
>- * fp->nvars to ngvars + nregexps.
>+ * fp->vars slot and to protect regexp slots from GC we set fp->nvars to
>+ * nregexps.

Since you are here, "slots" after "are stored in fp->vars", and a comma after "slots" and before "and to protect ...."

>+    /*
>+     * Adds a use of a variable that is statically known to exist on the
>+     * global object. The slot must be an index into cg->globalScope->defs.
>+     * If the global use can be cached, |cookie| will be set to |slot|.
>+     * Otherwise, |cookie| is set to the free cookie value.
>+     *
>+     * After compilation succeeds, all global uses have their slots rewritten
>+     * to point to the correct slot on the global object.

Good comments but still missing the why -- talk about lazy resolve-hooked bindings and COMPILE_N_GO global-code statement tree at a time emitting interacting badly if one tried to do everything while parsing.

>+     */
>+    bool addGlobalUse(JSAtom *atom, uint32 slot, js::UpvarCookie &cookie);

Mutable ref out params considered harmful. Use a pointer.

>+    while (worklist.length()) {
>+        JSScript *item = worklist.back();

Call this script or outer?

>+        JSFunctionBox *funbox = pn->pn_type == TOK_FUNCTION ? pn->pn_funbox : NULL;

Nit: paren the ? condition.

>+        if (prop) {
>+            AutoPropertyDropper dropper(cx, globalObj, prop);
>+
>+            /*
>+             * A few cases where we don't bother aggressively caching:
>+             *   1) Function value changes.
>+             *   2) Configurable properties.
>+             *   3) Properties without slots, or with getters/setters.
>+             */
>+            const Shape *shape = (const Shape *)prop;
>+            if (funbox ||

Does funbox mean a function value replaced a function value? Replacing a non-function might want optimization.

>+            if (!globalScope->defs.append(GlobalScope::GlobalDef(shape->slot)))
>+                return false;
>+        } else {
>+            if (!globalScope->defs.append(GlobalScope::GlobalDef(atom, funbox)))
>+                return false;
>+        }

Fuse common tail that calls append?

>+         * Functions can be redeclared, and the last one takes effect. Check
>+         * for this and make sure to rewrite the definition.
>+         *
>+         * Note: This could overwrite an existing variable declaration, for
>+         * example:
>+         *   var c = []
>+         *   function c() { }
>+         *
>+         * This rewrite is allowed because the function will be statically
>+         * hoisted to the top of the script, and the |c = []| will just
>+         * overwrite it at runtime.
>          */
>+        if (pn->pn_type == TOK_FUNCTION) {
>             uint32 index = ALE_INDEX(ale);
>+            globalScope->defs[index].funbox = pn->pn_funbox;
>+        }

pn_type TOK_FUNCTION occurs with other arities -- assert this is PN_FUNC.

>     struct GlobalDef {
>-        JSAtom *atom;
>-        JSFunctionBox *funbox;
>+        JSAtom *atom;           // If non-NULL, specifies the property name to add.
>+        JSFunctionBox *funbox;  // If non-NULL, function value for the property.
>+                                // This value is only set/used if atom is non-NULL.
>+        uint32 knownSlot;       // If atom is NULL, this is the known shape slot.

Nit, here and in other new structs: align member declarators (including * if any) for legibility.

r=me with these addressed. Thanks!

/be

Comment 44

[David Anderson [:dvander] - inactive, e-mail if emergency]

Pushed with comments addressed:

http://hg.mozilla.org/mozilla-central/rev/81c0aef6b272

Comment 45

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/47408f63366c