Last Comment Bug 622301 - Drop support for using DOM objects as prototypes of random objects and having DOM operations on the random object work on the DOM object instead
: Drop support for using DOM objects as prototypes of random objects and having...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 713747 726949 785822 788966 790303
Blocks: 622298
  Show dependency treegraph
 
Reported: 2010-12-31 15:41 PST by Boris Zbarsky [:bz]
Modified: 2012-09-11 14:33 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2 (2.34 KB, patch)
2012-01-03 12:06 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Remove now-obsolete 'callee' parameter. v1 (19.67 KB, patch)
2012-01-03 12:07 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5 (2.95 KB, patch)
2012-01-11 22:57 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Remove now-obsolete 'callee' parameter. v2 (19.52 KB, patch)
2012-01-11 22:57 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2010-12-31 15:41:33 PST
It's not the way prototypes are supposed to work in ES, and overcomplicates code significantly.  A good first step would be to stop looking up the proto chain for the XPCWrappedNative or slimwrapper in quickstubs; this should make the unwrapping code much simpler.
Comment 1 Boris Zbarsky [:bz] 2010-12-31 15:51:38 PST
One question here.  Right now, quickstub unwrapping claims that it "sees through XOWs, XPCNativeWrappers, and SafeJSObjectWrappers".

Does it need to continue to do this?
Comment 2 Blake Kaplan (:mrbkap) 2011-03-15 11:18:35 PDT
Yes it does. Hopefully that's not too expensive though.

(For the record, this is implemented by the IsSecurityWrapper and then Unwrap code in xpcquickstubs.cpp:getWrapper.)
Comment 3 Boris Zbarsky [:bz] 2011-12-06 12:43:02 PST
Bobby is throwing himself on this machine gun nest.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-12-22 23:56:30 PST
I've got some patches for this, though I think even more simplification can be done.

github branch: https://github.com/bholley/mozilla-central/commits/nonativeprotos
try push: https://tbpl.mozilla.org/?tree=Try&rev=d6326a7296ed
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-01-03 12:06:24 PST
Created attachment 585504 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2

So I've been trying to figure out a way to make this work for all of XPConnect, rather than just QuickStubs. But there's some trickiness with Xray XOWs, and after talking with Blake, it sounds like it makes the most sense to do that as a following bug.

In the mean time, here's the quickstubs version. Flagging mrbkap for review.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-01-03 12:07:12 PST
Created attachment 585505 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v1
Comment 7 Blake Kaplan (:mrbkap) 2012-01-05 03:30:54 PST
Comment on attachment 585504 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2

Review of attachment 585504 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCQuickStubs.cpp
@@ +789,5 @@
> +    // get something tangible.
> +    while (js::IsWrapper(obj)) {
> +        obj = XPCWrapper::Unwrap(cx, obj);
> +        if (!obj)
> +            return NS_ERROR_XPC_SECURITY_MANAGER_VETO;

Instead of a while loop, this could be two nested if statements (and then an assertion). We can have at most 1 security wrapper followed by (at most) an outer window. After that, we have to find a wrapped native.

Note that in the case of a CrossOrigin or NoWaiver wrapper, the "double wrapping" is taken care of in XPCWrapper::Unwrap.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-01-05 11:14:12 PST
(In reply to Blake Kaplan (:mrbkap) from comment #7)
> Note that in the case of a CrossOrigin or NoWaiver wrapper, the "double
> wrapping" is taken care of in XPCWrapper::Unwrap.

Is it? It looks to me like we only do one layer of unwrapping there...
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-01-06 15:24:18 PST
(In reply to Bobby Holley (:bholley) from comment #8)

> Is it? It looks to me like we only do one layer of unwrapping there...

Oh, I see. js::UnwrapObject actually recursively unwraps everything except for outer objects (in which case it only does one layer of unwrapping). I hadn't realized that.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-01-06 16:22:20 PST
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=4077fdcc109e

Blake, in case you're interested, the new version of the code we were discussing is as follows:

+    // We can have at most three layers in need of unwrapping here:
+    // * A (possible) security wrapper
+    // * A (possible) Xray waiver
+    // * A (possible) outer window
+    //
+    // The underlying call to js::Unwrap recursively unwraps, but stops if it
+    // hits an outer object. Thus, we need to make at most two unwrapping
+    // calls: one to handle security wrappers and waivers, and one to handle
+    // outer objects. 
+    if (js::IsWrapper(obj))
+        obj = XPCWrapper::Unwrap(cx, obj);
+    if (obj && js::IsWrapper(obj)) {
+        MOZ_ASSERT(js::Wrapper::wrapperHandler(obj)->isOuterWindow());
+        obj = XPCWrapper::Unwrap(cx, obj);
     }

-    *cur = obj;
+    // The safe unwrap might have failed for SCRIPT_ACCESS_ONLY objects. If it
+    // didn't fail though, we should be done with wrappers.
+    if (!obj)
+        return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
+    MOZ_ASSERT(!js::IsWrapper(obj));
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-01-06 17:32:41 PST
Landed to mozilla-inbound:

* http://hg.mozilla.org/integration/mozilla-inbound/rev/668a56be0eef
* http://hg.mozilla.org/integration/mozilla-inbound/rev/b1612e3ba9b9
Comment 12 Ed Morley [:emorley] 2012-01-07 03:59:37 PST
Could this be the cause of https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/-7aQ03BTo4k / was this expected? :-)
Comment 14 Ed Morley [:emorley] 2012-01-07 06:26:34 PST
Backed out for now so that the Dromaeo results over the coming hours can confirm either way :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/48da43d7924a
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-01-11 11:48:36 PST
I've been investigating this issue this week.

It looks like the actually dromaeo numbers are scattered, and there are some wins in some places. However we lose pretty hard on dom-traverse, where we go from ~460 to ~370.

With profiling, I noticed that the calls to js::IsWrapper are really hot, and that I actually introduced several more of them. The old hot path actually had no calls to js::IsWrapper, and my patch added 2 unconditional calls (the second one was silly, and should have been conditional on the first). So I filed bug 717113, and started to do optimization and inlining there.

But the numbers still weren't adding up. I looked into it again, and realized that it was actually my _second_ patch that was causing the major perf hit. The second patch only does one non-trivial thing, which is here: hg.mozilla.org/integration/mozilla-inbound/rev/b1612e3ba9b9#l4.68

Basically, the castNativeFromWrapper code just copy-pastes the central part of getWrapper without any of the checks, and bails out to getWrapper if that fails. I'm pretty flabbergasted that this was so hot, but it was.

I'm guessing it might have a lot to do with the function call overhead, so I'm going to try inlining getWrapper and see if that helps.
Comment 16 Peter Van der Beken [:peterv] - away till Aug 1st 2012-01-11 12:17:31 PST
(In reply to Bobby Holley (:bholley) from comment #15)
> Basically, the castNativeFromWrapper code just copy-pastes the central part
> of getWrapper without any of the checks, and bails out to getWrapper if that
> fails. I'm pretty flabbergasted that this was so hot, but it was.

I don't think I'd write the code like that if it didn't matter for performance.
Comment 17 Boris Zbarsky [:bz] 2012-01-11 12:21:50 PST
Inlining getWrapper is worth trying.  If we do that, we should also reorder it so that it checks for the common cases first, imo.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-01-11 22:57:29 PST
So by cleaning up a few things in the first patch and leaving the manual inlining in the second, I've got this to be more or less dromaeo-neutral: http://dromaeo.com/?id=160322,160547

So I'm going to try to land this as-is, and handle everything else in followup bugs. Patches coming right up.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-01-11 22:57:52 PST
Created attachment 587955 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-01-11 22:57:59 PST
Created attachment 587956 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v2
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-01-13 18:30:40 PST
Gave this one last push to try to double-check: https://tbpl.mozilla.org/?tree=Try&rev=6bb5a094065b
Comment 22 :Ms2ger 2012-01-14 00:40:03 PST
Caught a bad parent, so I pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=c5294fcc3baf
Comment 24 Phil Ringnalda (:philor, back in August) 2012-01-14 20:22:47 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b641c65643b9 for utterly opaque crashes in the Windows PGO profiling run like https://tbpl.mozilla.org/php/getParsedLog.php?id=8557108&tree=Mozilla-Inbound - I don't envy you the debugging of that, but I'm looking forward to how you'll probably have to leave automation.py/profileserver.py in better shape for the next person who hits a crash there ;)
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2012-02-07 12:40:33 PST
khuey fixed the PGO bug, and I believe the fix has landed on trunk.

Rebased the patches and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=082cbe8903da
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-02-07 18:07:51 PST
Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9dde014a4b3f
http://hg.mozilla.org/integration/mozilla-inbound/rev/b89b725e10fb

In theory the windows PGO build should be green this time. Let's see.
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2012-02-08 09:16:25 PST
bz pointed out that this was a very modest dromaeo win:

Improvement! Dromaeo (CSS) increase 2.52% on Linux Mozilla-Inbound-Non-PGO
http://mzl.la/xYKHEp
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2012-03-27 13:59:02 PDT
Backed out on aurora until we can get a patch together for bug 726949:

http://hg.mozilla.org/releases/mozilla-aurora/rev/1f489405617a
http://hg.mozilla.org/releases/mozilla-aurora/rev/98c38622905f

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