JS Error: Uncaught Exception using "for (key in array)" statements

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P2
major
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Byung Kim, Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
fixed1.8.1, regression, testcase, verified1.8.0.2
Points:
---
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rft-dl] not a moz1.7/ff1.0 problem, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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.
Assignee: nobody → general
Component: General → JavaScript Engine
Keywords: regression
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Not JS engine -- probably DOM, possibly XPConnect if not DOM.

/be
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM
Ever confirmed: true
So what key is it that causes the exception?  That is, can you catch the exception and see when it actually happens?
Keywords: qawanted
(Reporter)

Comment 4

13 years ago
(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)

Comment 5

13 years ago
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)
(Reporter)

Comment 6

13 years ago
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?
(Reporter)

Comment 8

13 years ago
I'm working on a reliable testcase. So far, I've isolated it to not being related to the js object structure.
(Reporter)

Comment 9

13 years ago
Created attachment 209034 [details]
Test case

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.
Blocks: 296639
Keywords: testcase
Attachment #209034 - Attachment mime type: application/octet-stream → application/zip
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.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 12

13 years ago
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.
Group: security
Flags: blocking1.8.0.1?
(Assignee)

Comment 16

13 years ago
Created attachment 209159 [details] [diff] [review]
Allow js to resolve standard classes

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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #209159 - Flags: superreview?(brendan)
Attachment #209159 - Flags: review?(jst)
(Assignee)

Comment 17

13 years ago
Created attachment 209161 [details] [diff] [review]
Allow js to resolve standard classes handling early returns

bz pointed my stupid mistake out.
Attachment #209159 - Attachment is obsolete: true
Attachment #209161 - Flags: superreview?(bzbarsky)
Attachment #209161 - Flags: review?(jst)
Attachment #209159 - Flags: superreview?(brendan)
Attachment #209159 - Flags: review?(jst)
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.
Attachment #209161 - Flags: review?(jst) → review+
(Assignee)

Updated

13 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 19

13 years ago
Created attachment 209171 [details] [diff] [review]
With jst's comment addressed

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.).
Attachment #209161 - Attachment is obsolete: true
Attachment #209171 - Flags: superreview?(brendan)
Attachment #209171 - Flags: review+
Attachment #209161 - Flags: superreview?(bzbarsky)
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
Attachment #209171 - Flags: superreview?(brendan) → superreview+
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
(Assignee)

Comment 22

13 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Comment on attachment 209171 [details] [diff] [review]
With jst's comment addressed

We should probably get this in on the branches...
Attachment #209171 - Flags: approval1.8.1?
Attachment #209171 - Flags: approval1.8.0.2?
Flags: blocking1.8.0.1? → blocking1.8.0.2?

Updated

13 years ago
Attachment #209171 - Flags: approval1.8.1? → branch-1.8.1?(jst)

Updated

13 years ago
Attachment #209171 - Flags: branch-1.8.1?(jst) → branch-1.8.1+

Updated

13 years ago
Flags: testcase+

Comment 24

13 years ago
BZ-Are you still looking for qawanted?

bclary- Can you capture the existing test case for regression testing?

Comment 25

13 years ago
(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.
Keywords: qawanted
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Comment on attachment 209171 [details] [diff] [review]
With jst's comment addressed

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #209171 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 28

13 years ago
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.2, fixed1.8.1

Updated

13 years ago
Whiteboard: [rft-dl]
Whiteboard: [rft-dl] → [rft-dl] not a moz1.7/ff1.0 problem
Verifed with Firefox 1.5.0.2 Windows build of 20060306
Keywords: fixed1.8.0.2 → verified1.8.0.2
Group: security

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.