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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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) (inactive)]

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 [:bzbarsky]]

Bobby is throwing himself on this machine gun nest.

Comment 4

[Bobby Holley (:bholley)]

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)]

Attachment: 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)]

Attachment: Part 2 - Remove now-obsolete 'callee' parameter. v1

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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)]

(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)]

(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)]

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)]

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]]

Could this be the cause of https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/-7aQ03BTo4k / was this expected? :-)

Comment 13

[Ed Morley [:emorley]]

Manual inspection of both: 
http://graphs-new.mozilla.org/graph.html#tests=[[72,131,15]]&sel=1325815778000,1325988578000&displayrange=7&datatype=running 
and 
http://graphs-new.mozilla.org/graph.html#tests=[[73,131,15]]&sel=1325815778000,1325988578000&displayrange=7&datatype=running 

...confirms the regression range as:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a1a4157bf49&tochange=a761ff40306b

Comment 14

[Ed Morley [:emorley]]

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)]

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]]

(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 [:bzbarsky]]

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)]

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)]

Attachment: Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove now-obsolete 'callee' parameter. v2

Comment 21

[Bobby Holley (:bholley)]

Gave this one last push to try to double-check: https://tbpl.mozilla.org/?tree=Try&rev=6bb5a094065b

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Caught a bad parent, so I pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=c5294fcc3baf

Comment 23

[Bobby Holley (:bholley)]

Pushed this to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/46ae4bf4aaf4
http://hg.mozilla.org/integration/mozilla-inbound/rev/3ab1dcfb2218

Comment 24

[Phil Ringnalda (:philor)]

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)]

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)]

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 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9dde014a4b3f
https://hg.mozilla.org/mozilla-central/rev/b89b725e10fb

Green PGO \o/

Comment 28

[Bobby Holley (:bholley)]

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)]

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