Last Comment Bug 457022 - Cache DOM wrappers in the DOM object
: Cache DOM wrappers in the DOM object
Status: RESOLVED FIXED
: fixed1.9.1, perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
P1 normal with 2 votes (vote)
: mozilla1.9.1b2
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
: 378389 (view as bug list)
Depends on: 464114 471395 CVE-2009-2665
Blocks: 461563 461566 474456
  Show dependency treegraph
 
Reported: 2008-09-25 10:28 PDT by Olli Pettay [:smaug]
Modified: 2009-10-28 14:33 PDT (History)
14 users (show)
jst: blocking1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (55.04 KB, patch)
2008-09-30 12:44 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1.1 (49.19 KB, patch)
2008-09-30 18:13 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v2 (48.86 KB, patch)
2008-10-24 06:23 PDT, Peter Van der Beken [:peterv]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
v2 (50.42 KB, patch)
2008-11-03 07:13 PST, Peter Van der Beken [:peterv]
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review
v2.1 (50.38 KB, patch)
2008-11-04 13:48 PST, Peter Van der Beken [:peterv]
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review
v2.2 (51.32 KB, patch)
2008-11-13 06:24 PST, Peter Van der Beken [:peterv]
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] 2008-09-25 10:28:29 PDT
Currently DOM Nodes' and XHRs' wrappers are stored in their owner document.
I'm not quite that is the right way. Perhaps scriptContext or global object
would be a better place?
Comment 1 User image Peter Van der Beken [:peterv] 2008-09-29 15:03:48 PDT
I've actually been working on a patch for the last week or two that stores them in the objects themselves. I need to update my tree for the recent XHR changes.
Comment 2 User image Olli Pettay [:smaug] 2008-09-29 15:30:41 PDT
(In reply to comment #0)
> I'm not quite that is the right way. 
+sure

Is it possible that we have several wrappers we should be able store?
Currently that isn't possible because node is the key
and wrapper the value in the hashtable.
Comment 3 User image Peter Van der Beken [:peterv] 2008-09-30 12:44:42 PDT
Created attachment 341136 [details] [diff] [review]
v1
Comment 4 User image Peter Van der Beken [:peterv] 2008-09-30 12:46:20 PDT
We should only have one wrapper for these objects.
Patch gives a 20-30% speedup on some of the dromaeo DOM tests.
Comment 5 User image Olli Pettay [:smaug] 2008-09-30 13:31:32 PDT
Comment on attachment 341136 [details] [diff] [review]
v1

>@@ -744,9 +842,19 @@ protected:
>    * NODE_* macros for the layout of the bits in this
>    * member.
>    */
>   PtrBits mFlagsOrSlots;
>+
>+  nsPreservedWrapper mWrapper;
> };
Could we store mWrapper in slots?
Comment 6 User image Peter Van der Beken [:peterv] 2008-09-30 17:31:35 PDT
Don't think that's necessary (see also https://bugzilla.mozilla.org/show_bug.cgi?id=453738#c27).
Comment 7 User image Peter Van der Beken [:peterv] 2008-09-30 18:13:19 PDT
Created attachment 341195 [details] [diff] [review]
v1.1
Comment 8 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-10-01 11:24:31 PDT
I wonder if it'd be worth adding a new class with no virtual methods and one member to point to the wrapper/JSObject? That way it'd be possible to QI to that class w/o doing any reference counting, and down the road we could maybe bake this in with the class we're considering adding for this pointer offsets etc as well.
Comment 9 User image Peter Van der Beken [:peterv] 2008-10-22 11:47:38 PDT
Comment on attachment 341195 [details] [diff] [review]
v1.1

Ugh, should have cleared those. I'll attach a newer version of the patch tomorrow.
Comment 10 User image Peter Van der Beken [:peterv] 2008-10-24 06:23:22 PDT
Created attachment 344619 [details] [diff] [review]
v2
Comment 11 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-10-28 17:21:43 PDT
Comment on attachment 344619 [details] [diff] [review]
v2

XPCWrappedNative::GetUsedOnly():

+    nsWrapperCache* cache;
+    Object->QueryInterface(NS_GET_IID(nsWrapperCache), (void**)&cache);
+    if(cache &&
+       (wrapper = static_cast<XPCWrappedNative*>(cache->GetWrapper())))
+    {
+        NS_ADDREF(wrapper);
+    }
+    else
+    {
         [do expensive stuff]

If the above QI succeeds, there's no point in going into the else case even if there is no wrapper in the cache since we won't find the wrapper in the map etc either.

r+sr=jst with that.
Comment 12 User image Peter Van der Beken [:peterv] 2008-11-03 07:13:01 PST
Created attachment 346037 [details] [diff] [review]
v2

Patch that was landed.
Comment 13 User image Peter Van der Beken [:peterv] 2008-11-03 07:41:46 PST
Backed out for unit test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225722086.1225726095.12145.gz&fulltext=1
Comment 14 User image Peter Van der Beken [:peterv] 2008-11-03 09:35:15 PST
The problem is xpcshell tests that create an XMLHttpRequest object, we'll have a null mOwner and so the PreCreate hook returns an error.
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2008-11-03 09:46:26 PST
Can't we just avoid changing parentObj in that case?
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2008-11-03 09:47:00 PST
And can we somehow assert (ideally statically) that the only way to get into that precreate method is with XHR?
Comment 17 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-11-03 10:07:29 PST
Yeah, I think I agree with bz here, given that this case does indeed happen. It'd been nice to not have to go there, but I don't see a better way to deal with that.
Comment 18 User image Peter Van der Beken [:peterv] 2008-11-04 13:48:41 PST
Created attachment 346327 [details] [diff] [review]
v2.1

Had to back out again, mailnews tinderboxes went orange. Current theory is that because their (hand-coded) QI implementations don't set result pointer to null on failure, we'll get a bogus pointer when doing

        nsWrapperCache* cache;
        CallQueryInterface(src, &cache);

I'm going to try to check in this patch (with |nsWrapperCache* cache = nsnull;|) early tomorrow morning.
Comment 19 User image Peter Van der Beken [:peterv] 2008-11-07 08:08:49 PST
Had to back out again, orange due to leaks. I found one of the causes, need to release wrapper when FindTearoff fails in XPCConvert::NativeInterface2JSObject, but there's still some leaks remaining.
Comment 20 User image Peter Van der Beken [:peterv] 2008-11-10 15:27:02 PST
Patch in bug 464114 fixes some of the leaks that show up. Still hunting more.
Comment 21 User image Peter Van der Beken [:peterv] 2008-11-12 10:05:58 PST
Actually, on trunk with the patch from bug 464114 this doesn't cause any additional leaks (borwser-chrome tests already leak on tinderbox). So this is now blocked on review and landing of the patch in bug 464114.
Comment 22 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-11-12 15:05:42 PST
We really should get this in for Beta2 as has always been the plan. The bug for some reason never got the flag.
Comment 23 User image Peter Van der Beken [:peterv] 2008-11-13 06:24:01 PST
Created attachment 347958 [details] [diff] [review]
v2.2

This is the patch that I'm going to land. It fixes any remaining leaks when running mochitests (apart from the one fixed in bug 464114).
Comment 24 User image Peter Van der Beken [:peterv] 2008-11-13 11:45:52 PST
Looks like it finally sticks. Filed bug 464735 for figuring out a better fix for the leaks.
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2009-01-20 12:19:01 PST
This seems to have caused at least one change in whether chrome sees XPCNativeWrappers.  See bug 471395.
Comment 26 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 23:29:14 PDT
Also apparently caused bug 498897
Comment 27 User image Peter Van der Beken [:peterv] 2009-10-28 14:33:22 PDT
*** Bug 378389 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.