Closed Bug 595917 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

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

People

(Reporter: danne.da, Assigned: dvander)

References

Details

(Keywords: regression, Whiteboard: [has reviewed patch])

Attachments

(3 files, 9 obsolete files)

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
For Reference:
Does setting javascript.options.methodjit.content;false and/or javascript.options.tracejit.content;false help?
Summary: D-Link router JavaScript navigation links broken since JM merge → JM: D-Link Router JavaScript Navigation Links broken since JM Merge
Version: unspecified → Trunk
Disabling either of those has no effect, nor does starting the browser in safe-mode.
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.
Duplicate of this bug: 596083
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
(In reply to comment #5)
It's already known that this is a JM Merge Regression - but thanks anyways.
Does this need to block the next beta?
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
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
Is it possible to attach the complete relevant content, HTML and JS and CSS?

/be
(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?
Attached file A Dlink router page for analysis. (obsolete) —
Attached image Matching Router page. (obsolete) —
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.
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
Keywords: testcase-wanted
Attached image Error Console Log (obsolete) —
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
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.
OS: Mac OS X → All
blocking2.0: ? → betaN+
Attached file Testcase (obsolete) —
A testcase, gives the same error as a router page.

No error shows up in a pre-JM build.
Attachment #476544 - Attachment mime type: text/html → text/plain
BTW, should the testcase be text/plain or text/html?
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.
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]
I don't get an error either on that link or locally.

Can you try with a clean profile, extensions disabled, cleared cache, etc?
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.
The test can be simplified even further, to this:

var req = new XMLHttpRequest();
req.open("GET", "data:,Hello%2C%20World!", false);
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.
Attached file ajax.js (obsolete) —
Attached file navigation.js (obsolete) —
Attached file utils.js (obsolete) —
Attached file Modified testcase (obsolete) —
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);
Attachment #476544 - Attachment is obsolete: true
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?)
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?
(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.
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?
And thanks to everyone for posting those scripts and reduced testcases!  That was a huge help in figuring out what's going on.
(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
This should probably block b7.

/be
blocking2.0: betaN+ → ?
Brendan, thanks for the spec-check.  :)  Blocking b7 it is.
blocking2.0: ? → beta7+
Assignee: general → dvander
Status: NEW → ASSIGNED
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.
Attached patch fix (obsolete) — Splinter Review
Attachment #476214 - Attachment is obsolete: true
Attachment #476215 - Attachment is obsolete: true
Attachment #476536 - Attachment is obsolete: true
Attachment #476548 - Attachment is obsolete: true
Attachment #476549 - Attachment is obsolete: true
Attachment #476550 - Attachment is obsolete: true
Attachment #476551 - Attachment is obsolete: true
Attachment #476919 - Flags: review?(brendan)
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.)
Duplicate of this bug: 598156
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
Attached patch commented fixSplinter Review
Attachment #476919 - Attachment is obsolete: true
Attachment #477006 - Flags: review?(brendan)
Attachment #476919 - Flags: review?(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
Attachment #477006 - Flags: review?(brendan) → review+
Whiteboard: [has reviewed patch]
Pushed with comments addressed:

http://hg.mozilla.org/mozilla-central/rev/81c0aef6b272
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.