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

VERIFIED FIXED

Status

()

VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: bc, Assigned: mrbkap)

Tracking

({regression})

Trunk
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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
Flags: in-testsuite+
I don't know if there would have been other possibilities but this bug is caused by bug 390001.
Blocks: 390001

Comment 2

12 years ago
offtopic: why did this not make the tinderbox orange?
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
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.
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
QA Contact: general → xpconnect
(Assignee)

Updated

11 years ago
Assignee: nobody → mrbkap
(Assignee)

Comment 7

11 years ago
Created attachment 275918 [details] [diff] [review]
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.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Depends on: 393306
Flags: blocking1.9?
(Assignee)

Comment 8

11 years ago
Created attachment 279648 [details] [diff] [review]
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?
Attachment #275918 - Attachment is obsolete: true
Attachment #279648 - Flags: review?(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
Attachment #279648 - Flags: review?(brendan)
Attachment #279648 - Flags: review+
Attachment #279648 - Flags: approval1.9+
(Assignee)

Comment 10

11 years ago
Created attachment 281017 [details] [diff] [review]
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.
Attachment #281017 - Flags: review?(brendan)
Comment on attachment 281017 [details] [diff] [review]
Interdiff fixing brendan's comments

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

/be
Attachment #281017 - Flags: review?(brendan)
Attachment #281017 - Flags: review+
Attachment #281017 - Flags: approval1.9+
(Assignee)

Comment 12

11 years ago
Created attachment 281030 [details] [diff] [review]
Full updated patch

This already has r+a=brendan, looking for sr.
Attachment #279648 - Attachment is obsolete: true
Attachment #281017 - Attachment is obsolete: true
Attachment #281030 - Flags: superreview?(jst)
Attachment #281030 - Flags: review+
(Assignee)

Comment 13

11 years ago
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.

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
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
Attachment #281030 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 15

11 years ago
I think newborn roots keep me safe here, but I'll change things so the memory management is more obvious.
(Assignee)

Comment 16

11 years ago
Created attachment 281966 [details] [diff] [review]
Interdiff fixing jst's comment

How's this?
Attachment #281966 - Flags: review?(jst)
Comment on attachment 281966 [details] [diff] [review]
Interdiff fixing jst's comment

Yeah, looks good to me.
Attachment #281966 - Flags: review?(jst) → review+

Comment 18

11 years ago
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?
(Assignee)

Comment 19

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 21

11 years ago
Created attachment 282941 [details] [diff] [review]
What was checked in
Attachment #281030 - Attachment is obsolete: true
Attachment #281966 - Attachment is obsolete: true
(Assignee)

Comment 22

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

11 years ago
verified fixed 1.9.0 win/mac*/linux
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.