User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 With Firefox 1.5, I'm seeing exceptions being thrown when using for-in loops. This is an example of an exception returned: Error: uncaught exception: [Exception... "Unexpected error" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: http://www.oldnavy.com/js/browse/product/en/productModelFunctions.js :: anonymous :: line 84" data: no] The line that the error is referring to is: for (var i in objV.arrayVariantStyleColors) { When I convert the statement to for(i=0;i<objV.arrayVariantStyleColors.length;i++) the error goes away. The problem is, not all my arrays are keyed by integers. I have some that are keyed by strings (i.e. someArray["someKey"]). Each time I modify a for-in loop to a for loop, the error occurs at the next for-in loop in the code flow. I have tried commenting out all of the statements within the for loop and added an alert statement to alert the key and value. The exception occurs before the alert can execute which tells me that its the loop itself and not what I'm doing with the elements in the array. Once the error manifests itself, you can repeat the action and the error doesn't come up again until you reload the page. The error also doesn't happen in IE or Safari. Reproducible: Always Steps to Reproduce: 1. Go to the URL provided. 2. Click on the "Tall" tab. Wait for it to load. 3. Click on the "Regular" Tab and wait for that to load. 4. Click on the "Tall" tab. Check the Javascript Console for the error. 5. Click again on the "Tall" tab and it works without errors. Another Example: 1. Go to the URL provided 2. Click on the "Outfit" tab. Wait for it to load. 3. Click on the "Overview" tab. Error in the Javasript Console. (The error points to another for-in loop in a different area of the js codeset. 4. Click on "Overview" again and it works properly without errors. With the first set of steps, try the following: 1. Reload the page 2. Paste into the URL the following (should be all one line): javascript:alert(objPO.initializeColors = function() {objPO.selectedColor = 0;if (objPO.strDefaultStyleColorId != -1) {for (i=0;i<objV.arrayVariantStyleColors.length;i++) {alert(i + " " + objV.arrayVariantStyleColors[i]);if (objV.arrayVariantStyleColors[i].strColorCodeId == objPO.strDefaultStyleColorId)objPO.selectedColor = i;}}if (objPO.objCookieData.color) {for (i=0;i<objV.arrayVariantStyleColors.length;i++) {alert(i + " " + objV.arrayVariantStyleColors[i]);if (objV.arrayVariantStyleColors[i].strColorName == objPO.objCookieData.color) objPO.selectedColor = i;}}if (objPO.selectedColorName != "") {for (i=0;i<objV.arrayVariantStyleColors.length;i++) {alert(i + " " + objV.arrayVariantStyleColors[i]);if (objV.arrayVariantStyleColors[i].strColorName == objPO.selectedColorName)objPO.selectedColor = i;}}objPO.activeColor = objPO.selectedColor;objPO.selectedColorName = objV.arrayVariantStyleColors[objPO.selectedColor].strColorName;}); This will override the function and convert the for-in loops to for loops as well as alert the key and value of each element in the array to demonstrate that there are no bad elements within the array. 3. Repeat the first set of steps above and go through the alerts. When you reach step 4, there will be an exception but in a different area of the code. This exception points to a different for-in loop in a different section of the code set. Actual Results: for-in loops cause uncaught exceptions. Expected Results: for-in loops should execute without exceptions.

This was mentioned in this topic: http://forums.mozillazine.org/viewtopic.php?t=366723&highlight= It was also mentioned that this used to work fine in Firefox1.0.x builds.

Not JS engine -- probably DOM, possibly XPConnect if not DOM. /be

So what key is it that causes the exception? That is, can you catch the exception and see when it actually happens?

(In reply to comment #3) > So what key is it that causes the exception? That is, can you catch the > exception and see when it actually happens? > It's not any key that's causing the problem. I have tried commenting out the statements within the for-in loop and added 1 alert statement. The error occurs before the loop even beings to iterate. When I change the statement to a regular for loop, the alerts function properly and the loop completes. I get what I expect as the keys (integers) and I get the expected values (objects)

Sounds like the issue is in property enumeration logic. That's the only thing that occurs in the one for that's having problems. Sounds like it's having problems enumerating the object arrayVariantStyleColors. It might be telling if the for was changed it to: var colors = objV.arrayVariantStyleColors; for (var i in colors)

I tried modifying the statement as you suggested. The error still occurs.

OK. Any chance of a reasonably-sized testcase that could be usefully debugged?

I'm working on a reliable testcase. So far, I've isolated it to not being related to the js object structure.

I have isolated the error and made it readily reproducable in the attachment. Load bug323833.html in Firefox 1.5 and read through the description to understand what's happening.

Byung, thanks for the testcase! This regressed between 2005-07-30 and 2005-07-31, so very likely a regression from bug 296639.

So here's what I've found so far. We end up creating a new object (not sure why; doesn't so much matter), which makes us look up the Object constructor on the window (which window? Getting there). We end up calling nsWindowSH::NewResolve and get the stack: #0 nsWindowSH::AddProperty (this=0x87a8fa8, wrapper=0xb41e48b8, cx=0xb3e94f00, obj=0x87e0270, id=136377484, vp=0xbfffd18c, _retval=0xbfffd0dc) at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4352 #1 0xb79c72c7 in XPC_WN_Helper_AddProperty (cx=0xb3e94f00, obj=0x87e0270, idval=136377484, vp=0xbfffd18c) at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:928 #2 0xb7e23c2b in js_DefineNativeProperty (cx=0xb3e94f00, obj=0x87e0270, id=136387144, value=142477128, getter=0xb79c73e4 <XPC_WN_Helper_GetProperty>, setter=0xb79c74ca <XPC_WN_Helper_SetProperty>, attrs=0, flags=0, shortid=0, propp=0x0) at ../../../mozilla/js/src/jsobj.c:2537 #3 0xb7e23861 in js_DefineProperty (cx=0xb3e94f00, obj=0x87e0270, id=136387144, value=142477128, getter=0, setter=0, attrs=0, propp=0x0) at ../../../mozilla/js/src/jsobj.c:2423 #4 0xb7df5bdd in js_DefineFunction (cx=0xb3e94f00, obj=0x87e0270, atom=0x8211a48, native=0xb7df45fa <Function>, nargs=1, attrs=0) at ../../../mozilla/js/src/jsfun.c:2087 #5 0xb7dc003b in JS_InitClass (cx=0xb3e94f00, obj=0x87e0270, parent_proto=0x0, clasp=0xb7e89f80, constructor=0xb7df45fa <Function>, nargs=1, ps=0xb7e89f20, fs=0xb7e89fe0, static_ps=0x0, static_fs=0x0) at ../../../mozilla/js/src/jsapi.c:2034 #6 0xb7df554e in js_InitFunctionClass (cx=0xb3e94f00, obj=0x87e0270) at ../../../mozilla/js/src/jsfun.c:1965 #7 0xb7dbe70d in InitFunctionAndObjectClasses (cx=0xb3e94f00, obj=0x87e0270) at ../../../mozilla/js/src/jsapi.c:1127 #8 0xb7dbefab in JS_ResolveStandardClass (cx=0xb3e94f00, obj=0x87e0270, id=136377516, resolved=0xbfffd4b4) at ../../../mozilla/js/src/jsapi.c:1419 #9 0xb673418f in nsWindowSH::NewResolve (this=0x87a8fa8, wrapper=0xb41e48b8, cx=0x87a4dc0, obj=0x87e0270, id=136377516, flags=16, objp=0xbfffd554, _retval=0xbfffd558) at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:5733 Now addProperty does a security check. This security check fails with NS_ERROR_DOM_XPCONNECT_ACCESS_DENIED. So we throw all the way back up to where we started... Now the window we're trying to add a property to is an inner window. The corresponding outer window's document has "about:blank" as a URI. The inner window itself has a null mDocument. Furthermore, we have: (gdb) p win->mOuterWindow->mInnerWindow $70 = (nsPIDOMWindow *) 0xb41a9ec0 (gdb) p (nsPIDOMWindow*)win $72 = (nsPIDOMWindow *) 0xb41d73e8 (this is in AddProperty). So we're trying to add a property to the _old_ inner window... Brendan, shaver, any idea why we're doing that? In any case, when we get into nsScriptSecurityManager::CheckPropertyAccessImpl we end up calling: #0 nsScriptSecurityManager::CheckSameOriginDOMProp (this=0x8246e90, aSubject=0xb3ea19b8, aObject=0x829cde8, aAction=2, aIsCheckConnect=0) at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:902 (note that the two principals are not the same). Further: (gdb) p ((class nsPrincipal*)aObject)->mJSPrincipals->codebase $91 = 0x829cfe8 "file://" (gdb) p ((class nsPrincipal*)aSubject)->mJSPrincipals->codebase $92 = 0xb41d4c00 "about:blank" and hence access is disallowed. I'm a little confused as to why the subject here is about:blank too, for what it's worth... I guess that's the cx that we're using (the JSContext corresponding to the subframe). I'm just not sure _why_ that's the cx we're using. Oh, I see this on Linux too.

Here are some answers, though I'm not sure how to fix this, yet: So the reason we create a new object is that we try to create an iterator object to hold our enumeration state in. That object is created in |obj|'s scope, but in this case, obj is |a|, from the old iframe window, so we try to look the constructor up in that object's global object, which is the old inner. I guess we've already cleared the scope on the old inner window at this point, so it tries to add the property iterator's constructor to it, which is causing the bad security check. The reason that the subject principal is about:blank is because we're resolving on the old inner window. We try to get the subject principal off of its context, but its context has no stack frames, so we look at that context's global object, which is the outer window. I guess this is the problem, before splitwindow, the global object in a given cx would always have the same principals, now they change. Am I missing anything?

#9 0xb673418f in nsWindowSH::NewResolve (this=0x87a8fa8, wrapper=0xb41e48b8, cx=0x87a4dc0, obj=0x87e0270, id=136377516, flags=16, objp=0xbfffd554, _retval=0xbfffd558) at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:5733 What called that, pray tell? Unwind further to the js_Interpret frame that runs in the old inner window object (obj=0x87e0270) and things should be clearer. It seems a script *is* running using that old inner as the last object on its scope chain (probably the only object on its scope chain -- as you would expect for a top-level script using no with statements). /be

> before splitwindow, the global object in a given cx would always have the same > principals, now they change. Am I missing anything? Actually, before splitwindow the global object's principal's would change... but so would the principals of |a| (since its parent chain terminated at the window). So the principals would be the same, etc. This was a bug, of course. ;) > What called that, pray tell? #10 0xb7dffd65 in js_Interpret (cx=0x8844ce8, pc=0xb1a8e0c5 "\v", result=0xbfffdf40) at ../../../mozilla/js/src/jsinterp.c:2430 #11 0xb7dfcd5d in js_Execute (cx=0x8844ce8, chain=0xb1a09260, script=0x89405e8, down=0x0, flags=0, result=0xbfffe044) at ../../../mozilla/js/src/jsinterp.c:1480 #12 0xb7dc616e in JS_EvaluateUCScriptForPrincipals (cx=0x8844ce8, obj=0xb1a09260, principals=0x829ffec, chars=0xbfffe0f8, length=8, filename=0xbfffe2f8 "javascript:testIt()", lineno=1, rval=0xbfffe044) at ../../../mozilla/js/src/jsapi.c:4107 #13 0xb66a574f in nsJSContext::EvaluateString (this=0x8844c38, aScript=@0xbfffe0e0, aScopeObject=0xb1a09260, aPrincipal=0x829ffe8, aURL=0xbfffe2f8 "javascript:testIt()", aLineNo=1, aVersion=0x0, aRetValue=0xbfffe240, aIsUndefined=0xbfffe18c) at ../../../../mozilla/dom/src/base/nsJSEnvironment.cpp:1067 #14 0xb670dd79 in nsJSThunk::EvaluateScript (this=0x8800578, aChannel=0x8997c78) at ../../../../mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp:278 #15 0xb670ebe9 in nsJSChannel::InternalOpen (this=0x892ea30, aIsAsync=1, aListener=0x88f1ff8, aContext=0x0, aResult=0x0) at ../../../../mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp:538 #16 0xb670eae2 in nsJSChannel::AsyncOpen (this=0x892ea30, aListener=0x88f1ff8, aContext=0x0) at ../../../../mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp:509 #17 0xb5954ab7 in nsURILoader::OpenURI (this=0x8302348, channel=0x892ea30, aIsContentPreferred=1, aWindowContext=0x8851fe8) at ../../../mozilla/uriloader/base/nsURILoader.cpp:881 That's the link we clicked. In js_Interpret, we have: (gdb) p *cx->fp->script $5 = {code = 0xb1a8e0c0 "\001;", length = 16, main = 0xb1a8e0c0 "\001;", version = 0, numGlobalVars = 0, atomMap = {vector = 0xb1a8e0e0, length = 1}, filename = 0xb1a8f7e9 "file:///home/bzbarsky/test/bug323833/bug323833.html", lineno = 8, depth = 3, trynotes = 0x0, principals = 0x829ffec, object = 0x0} So the JS we're running there is running in the big window, not in the iframe.

So the issue here is that when we pass a JSContext to JS_ResolveStandardClass we don't pass the |cx| that got passed to NewResolve (quite purposefully). But when doing cross-window access there's going to be no script running on my_cx, and hence we'll always end up using the principals of the outer here. That's unfortunate. I guess my first question is whether it's ok to set sDoSecurityCheckInAddProperty to false while calling JS_ResolveStandardClass. Is that safe? If not, then we need to keep track of the "right" cx to pass to the security check, I guess. Time for another static? ;) Given that this does the wrong security check for cross-window access, I think we really ought to fix this on 1.8 too.

It's not possible for someone evil to replace one of the standard classes (or we wouldn't be doing a resolution), so not doing the security check is safe here. I still think that contexts should be a per-inner-window thing because of this case, but jst argues that that would bloat things quite a bit. Therefore, if we wanted to fix this the "right" way, bz's static would be the way to go. I propose to fix this bug by checking this fix into both the branch and the trunk, and adding the static in another bug to fix this for real.

bz pointed my stupid mistake out.

Comment on attachment 209161 [details] [diff] [review] Allow js to resolve standard classes handling early returns JSBool did_resolve = JS_FALSE; + PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; + sDoSecurityCheckInAddProperty = PR_FALSE; if (!::JS_ResolveStandardClass(my_cx, obj, id, &did_resolve)) { + sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; *_retval = JS_FALSE; return NS_ERROR_UNEXPECTED; } + sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; + I'd rather see this written as: sDoSecurityCheckInAddProperty = PR_FALSE; JSBool ok = ::JS_ResolveStandardClass(my_cx, obj, id, &did_resolve); sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; if (!ok) { ... This way it's harder for others to accidentally forget to do the cleanup... r=jst either way tho.

This also preserves the exception thrown by the call to JS_ResolveStandardClass (and makes sure to throw an exception in the case that nobody did) (and clears the pending exception on the old cx, etc.).

Comment on attachment 209171 [details] [diff] [review] With jst's comment addressed >+ JS_ClearPendingException(my_cx); >+ JS_SetPendingException(cx, exn); Slight preference for transposing these lines, although there's no GC hazard. As you wrote it, a ref-counting implementation, or a GC that could run on another thread at arbitrary instruction boundaries, or a GC that could nest in JS_SetPendingException before exn was "homed" (but the last sounds like a bug in JS_SetPendingException) all would suffer Bad Things here. sr=me with that. /be

Comment on attachment 209171 [details] [diff] [review] With jst's comment addressed >+ JS_ClearPendingException(my_cx); >+ JS_SetPendingException(cx, exn); Blake pointed out that my_cx could == cx, so these can't be transposed without an extra test. D'oh. Worth commenting. /be

Fix checked into trunk.

Comment on attachment 209171 [details] [diff] [review] With jst's comment addressed We should probably get this in on the branches...

BZ-Are you still looking for qawanted? bclary- Can you capture the existing test case for regression testing?

(In reply to comment #24) > BZ-Are you still looking for qawanted? > > bclary- Can you capture the existing test case for regression testing? > Already done. See testcase+ above. If you want a specific testcase added, request testcase? I am working on testcases now, no need to request specific ones for a couple of days until I have a chance to work through the list.

Nope, not qawanted anymore.

Comment on attachment 209171 [details] [diff] [review] With jst's comment addressed approved for 1.8.0 branch, a=dveditz for drivers

Fix checked into the 1.8 branches.

Verifed with Firefox 1.5.0.2 Windows build of 20060306