390947 - various js tests fail in browser due to unexpected DOM method calls

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bob Clary [:bc] (inactive)]

The following tests regressed in the browser between 8/1-8/2:

ecma_3/Array/15.4.4.4-001.js 
  nsIDOM3Document.domConfig NS_ERROR_NOT_IMPLEMENTED
js1_5/Regress/regress-233483-2.js  
  Permission denied to call method History.item
js1_6/extensions/regress-312385-01.js 
  Permission denied to call method History.item

Comment 1

[Ria Klaassen (not reading all bugmail)]

I don't know if there would have been other possibilities but this bug is caused by bug 390001.

Comment 2

[Bernd]

offtopic: why did this not make the tinderbox orange?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

The tinderboxes don't run the JS tests, only mochi- and reftests.

Comment 4

[Bernd]

:-(, I had a few days ago a discussion  with bz where I assumed that these tests do run on tinderbox.

Comment 5

[Ray Kiddy]

Note that there are tests which do not run on tinderbox, tests which do run but whose output is ignored, and tests which run and whose output can trigger tinderbox color changes.

There have been things that ran on tbox and their output was ignored, though that may not be true now. Just be wary of it.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Back on topic: this is going to suck a little bit to fix.

The problem is that, in bug 390001, we started to use the enumerate hook to enumerate all properties down the prototype chain. This fixed constructs like:

  for (i in location)
    foo[i] = location[i]

but it means that a call to JS_Enumerate, which should only return a shallow enumeration of the current object, will return all properties on the prototype chain. This bites us when MarkSharpObjects (called from Array.prototype.toString) tries to get every property returned from JS_Enumerate, which includes (and you guessed it) domConfig.

I'm going to try to fix this by overriding __iterator__ on XOWs and returning an enumeration object that does a "deep" iteration. This way, we'll get the best of both worlds.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rough cut

This isn't ready for review yet. I still have to stick the __iterator__ function somewhere useful and share it for all XOWs.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This uses the iteration hook added in bug 393306 (which I'm going to check in rsn). Brendan, I'm not sure that I got the keysonly stuff right, would you double-check that?

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 279648 [details] [diff] [review]
patch v1

>+  uintN idx = JSVAL_TO_INT(v);
>+
>+  if ((signed)idx == ida->length) {

Use jsint, avoid a (signed) cast. I don't see downside -- what am I missing?

>+    // Throw StopIteration.
>+    if (!JS_GetClassObject(cx, obj, JSProto_StopIteration, &obj)) {
>+      return JS_FALSE;
>+    }
>+
>+    JS_SetPendingException(cx, OBJECT_TO_JSVAL(obj));

Please instead make a JS_ThrowStopIteration that calls js_ThrowStopIteration and use that new JS API.

>+    ok = OBJ_LOOKUP_PROPERTY(cx, wrapperObj, ida->vector[i], &pobj, &prop);
>+    if (!ok) {
>+      break;
>+    }
>+
>+    if (pobj != wrapperObj) {
>+      ok = OBJ_DEFINE_PROPERTY(cx, wrapperObj, ida->vector[i], JSVAL_VOID,
>+                               nsnull, nsnull, JSPROP_ENUMERATE | JSPROP_SHARED,
>+                               nsnull);
>+    }
>+
>+    if (prop) {
>+      OBJ_DROP_PROPERTY(cx, pobj, prop);
>+    }

Drop prop (unlocking pobj) before conditionally defining a property on wrapperObj to avoid nesting object (scope) locks.

Looks good otherwise, how's it work? r+a=me carries over to a patch with fixes.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff fixing brendan's comments

Note that js_ThrowStopIteration never used its 'obj' parameter. I think it's cleaner and safer to just pass it a cx and let it use the scope chain anyway, so I did that.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 281017 [details] [diff] [review]
Interdiff fixing brendan's comments

Great, good catch no cx only parameterization for js_Throw*.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Full updated patch

This already has r+a=brendan, looking for sr.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 281030 [details] [diff] [review]
Full updated patch

>diff --git a/js/src/xpconnect/src/XPCWrapper.cpp b/js/src/xpconnect/src/XPCWrapper.cpp
>+    if (pobj != wrapperObj) {

This should really be (prop && pobj != wraperObj) if the resolve hook decided that the property shouldn't be reflected. I've made this change locally.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 281030 [details] [diff] [review]
Full updated patch

- In XPC_XOW_Iterator():

+  JSObject *wrapperIter = JS_NewObject(cx, &sXPC_XOW_JSClass.base, nsnull,
+                                       JS_GetGlobalForObject(cx, obj));
+  if (!wrapperIter) {
+    return nsnull;
+  }
+
+  JSObject *iterObj = JS_NewObject(cx, &IteratorClass, wrapperIter, obj);
+  if (!iterObj) {
+    return nsnull;
+  }
+
+  // Do this sooner rather than later to avoid complications in
+  // IteratorFinalize.
+  if (!JS_SetReservedSlot(cx, iterObj, 0, PRIVATE_TO_JSVAL(nsnull))) {
+    return nsnull;
+  }
+
+  // Root iterObj now. Note that this also protects wrapperIter.
+  JSAutoTempValueRooter tvr(cx, OBJECT_TO_JSVAL(iterObj));

Aren't you running a risk of having wrapperIter GCd here between creating it and creating iterObj? I.e. shouldn't you create iterObj, root it then create wrapperIter and stick it in iterObj's reserved slot etc?

With that answered, sr=jst

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I think newborn roots keep me safe here, but I'll change things so the memory management is more obvious.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff fixing jst's comment

How's this?

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 281966 [details] [diff] [review]
Interdiff fixing jst's comment

Yeah, looks good to me.

Comment 18

[cmtalbert]

Has this landed yet?  I'm still seeing this domConfig crash on the latest Trunk.
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007092504 Minefield/3.0a9pre

Also, would fixing this also fix the issue in bug 226193?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #18)
> Has this landed yet?  I'm still seeing this domConfig crash on the latest
> Trunk.

Not yet. I'll try to land it tonight if the tree opens again. Also, are you seeing a crash here or simply an unexpected exception?

> Also, would fixing this also fix the issue in bug 226193?

No, it won't. This bug fixes unexpected accesses to the domConfig property, not the underlying cause.

Comment 20

[cmtalbert]

(In reply to comment #19)
> (In reply to comment #18)
> > Has this landed yet?  I'm still seeing this domConfig crash on the latest
> > Trunk.
> 
> Not yet. I'll try to land it tonight if the tree opens again. Also, are you
> seeing a crash here or simply an unexpected exception?
> 
Sorry, I'm seeing an unexpected exception which causes a website to stop loading (renkoo.com (view invitations)).  The website uses the dojo toolkit framework, and it appears that something dojo is doing is causing the domConfig expception.  Sorry for being unclear. 

> > Also, would fixing this also fix the issue in bug 226193?
> 
> No, it won't. This bug fixes unexpected accesses to the domConfig property, not
> the underlying cause.
> 
Ah, ok.  Thanks for clarifying.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What was checked in

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 23

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 win/mac*/linux