399587 - deleting a property from an object doesn't notify that object's XOWs

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-68498-003.js now fails with

Testing calling obj.eval(str); currently at expect[3] within test - expected: false actual: true

while prior to bug 383682, it failed with
Testing calling obj.eval(str); currently at expect[1] within test - expected: 43 actual: false

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Ugh, it appears that 'delete' on a XOW is broken.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, that was an overreaction. The problem is that 'delete y' deletes y from the window object, but not the XOW. We need to notify the XOW of things like this, but need to wait until we can notify all XOWs.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Should probably fix this before 1.9.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

P4 sounds low for this. Doesn't this break any uses of |delete foo;| where foo is a global property for content? Not that delete is super common, but i'd assume it's used.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This patch fixes this bug, contains the fix for the orange caused by the patch in bug 393269, and paves the way for storing the object principal in the XOWs themselves instead of computing it every time. I haven't run it through the mochitests yet (will do that in a couple of minutes) but everything seems to be working.

The one real different change in this patch is that we now clamp XOWs parents much more tightly, but this should be a good thing (any objects that didn't follow this before were liable to get us in trouble).

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 294798 [details] [diff] [review]
Fix

- In xpcmaps.h:

+    struct Link : public PRCList
+    {
+        JSObject *datum;

Maybe name this "obj" to make the name more familiar? :)

+    inline JSObject* Add(WrappedNative2WrapperMap* head,
+                         JSObject* wrappedWrapper,
+                         JSObject* wrapper)

This method isn't all that small, so maybe not worth inlining this method. Also, maybe rename wrappedWrapper to wrappedObj?

+        Entry* entry = (Entry*)
+            JS_DHashTableOperate(mTable, wrappedWrapper, JS_DHASH_ADD);
+        if(!entry)
+            return nsnull;
+        NS_ASSERTION(!entry->key || this == head, "dangling pointer?");
+        entry->key = wrappedWrapper;
+        Link* l = new Link;

Seems like we could save on heap traffic here if Entry here would contain a Link member (not a pointer to one). You'll need to deal with entries moving in the hash etc, but shouldn't be too bad.

r- for now, but this is looking good over all.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

So, after running the mochitests, it became apparent that while this patch fixes one cause of the orange in bug 393269, that it's not quite fully fixed. The problem is related to the one described in bug 393269, comment 13 in that it involves document.open. The patch here causes us to clearscope on all of the XOWs for the moving document. However, because the document is moving, the XOW's "canonical" scope for finding XOWs for the document is also moving. Because of this, the *next* time the document's scope changes, we can't find the XOWs in nsXPConnect::UpdateXOWs and end up in the same situation as before.

I haven't actually tested this, so it might not be completely right, but it fits the symptoms and some debugging code I've tested. I'm working on fixing it now.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fixing that problem confirmed my hypothesis, updated patch, with mochitest coming up.

(In reply to comment #6)
> Seems like we could save on heap traffic here if Entry here would contain a
> Link member (not a pointer to one). You'll need to deal with entries moving in
> the hash etc, but shouldn't be too bad.

I'd rather put this off into another bug. This bug is doing enough that separating the optimization (which probably won't have too large an impact) seems reasonable to me.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

This addresses everything except for jst's last comment.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

By the way, the patch also moves the JS_ClearScope from nsHTMLDocument::Open into the place where we actually move the wrapper from the old scope to the new one.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 294864 [details] [diff] [review]
Updated to comments

- In nsWindowSH::DelProperty():

+  // Notify any XOWs on our outer window.

Would we want to do this for all XOWs? If so, we might be able to put this in XPConnect's wrapped natives delProperty jsops hooks...

- In WrappedNative2WrapperMap::AddLink():

+    oldLink->next->prev = newLink;
+    newLink->next = oldLink->next;
+    oldLink->prev->next = newLink;
+    newLink->prev = oldLink->prev;
+    PR_INIT_CLIST(oldLink);

This won't do the right thing if oldLink is an empty list. Maybe you could use PR_INSERT_LINK() to insert newLink after oldLink, and then PR_REMOVE_AND_INIT_LINK() to remove oldLink. That ought to do the right thing in all cases.

Leaving the optimization for a new bug is fine with me.

r+sr=jst with that.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> Would we want to do this for all XOWs? If so, we might be able to put this in
> XPConnect's wrapped natives delProperty jsops hooks...

This shouldn't be necessary because it should be impossible to address the property without going through the XOW.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I filed bug 410223 on avoiding allocating Link object separately.

Comment 14

[Mike Schroepfer]

There's a pretty big Private Bytes Regression

http://graphs.mozilla.org/#spst=range&spss=1198976687.2064924&spse=1199005000.5734897&spstart=1196883676&spend=1199299024&bpst=cursor&bpsc=1198967975.4012623&bpstart=1198976687.2064924&bpend=1199005000.5734897&m1tid=53258&m1bl=0&m1avg=0

This and bug 393267 are the only checkins during that time.  Possible this increased memory usage?

Comment 15

[Reed Loden [:reed]]

(In reply to comment #14)
> There's a pretty big Private Bytes Regression
> 
> This and bug 393267 are the only checkins during that time.  Possible this
> increased memory usage?

Bug 410291 ?

Comment 16

[Jesse Ruderman]

Attachment: patch to back this out

I'm planning to back this out to fix bug 410323 (which is a dogfood blocker, and seems to be turning Windows orange intermittently.)

I resolved two merge conflicts:
* nsIXPConnect.idl had a UUID conflict with bug 409433.  Firebot made me a new UUID.
* nsXPConnect.cpp had a nearby-code-touched conflict with bug 409433.

Comment 17

[Jesse Ruderman]

Backout patch checked in.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

I checked this back in now that bug 410323 is fixed.

Comment 19

[Mike Schroepfer]

Private bytes regression is back with this checkin:

http://graphs.mozilla.org/#spst=range&spss=1199498047.2488728&spse=1199512372.5707846&spstart=1196883676&spend=1199531473&bpst=Cursor&bpstart=1199498047.2488728&bpend=1199512372.5707846&m1tid=53258&m1bl=0&m1avg=0

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac/win

Comment 21

[Mike Schroepfer]

http://graphs.mozilla.org/#spst=range&spss=1199403445.2975655&spse=1199526778.0838594&spstart=1196881975&spend=1199921443&bpst=Cursor&bpstart=1199403445.2975655&bpend=1199526778.0838594&m1tid=53222&m1bl=0&m1avg=0&m2tid=53258&m2bl=0&m2avg=0&m3tid=53240&m3bl=0&m3avg=0&m4tid=53280&m4bl=0&m4avg=0

Mh regression still there bkap...

Comment 22

[Bob Clary [:bc] (inactive)]

(In reply to comment #21)

> Mh regression still there

shrep, Isn't that already covered by bug 410291 and can't this bug be still verified fixed even though there is an outstanding regression ?

Comment 23

[Mike Schroepfer]

Yep - sorry - restoring state