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

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: bholley)

Tracking

Trunk
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
One question here.  Right now, quickstub unwrapping claims that it "sees through XOWs, XPCNativeWrappers, and SafeJSObjectWrappers".

Does it need to continue to do this?
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.)
Bobby is throwing himself on this machine gun nest.
Assignee: nobody → bobbyholley+bmo
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
Depends on: 713747
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.
Attachment #585504 - Flags: review?(mrbkap)
Attachment #585504 - Attachment description: Bug 622301 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2 → Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v2
Created attachment 585505 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v1
Attachment #585505 - Flags: review?(mrbkap)
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.
Attachment #585504 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #585505 - Flags: review?(mrbkap) → review+
(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...
(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.
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));
Landed to mozilla-inbound:

* http://hg.mozilla.org/integration/mozilla-inbound/rev/668a56be0eef
* http://hg.mozilla.org/integration/mozilla-inbound/rev/b1612e3ba9b9
Target Milestone: --- → mozilla12
Could this be the cause of https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/-7aQ03BTo4k / was this expected? :-)
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
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
Target Milestone: mozilla12 → ---
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.
(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.
Inlining getWrapper is worth trying.  If we do that, we should also reorder it so that it checks for the common cases first, imo.
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.
Created attachment 587955 [details] [diff] [review]
Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5
Created attachment 587956 [details] [diff] [review]
Part 2 - Remove now-obsolete 'callee' parameter. v2
Attachment #587955 - Attachment description: Bug 622301 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5 → Part 1 - Don't use XPCWrappedNative::GetWrappedNativeOfJSObject in quickstub unwrapping. v5
Attachment #587956 - Attachment description: Bug 622301 - Remove now-obsolete 'callee' parameter. v2 → Part 2 - Remove now-obsolete 'callee' parameter. v2
Attachment #587956 - Flags: review?(mrbkap)
Attachment #587955 - Flags: review?(mrbkap)
Attachment #585504 - Attachment is obsolete: true
Attachment #585505 - Attachment is obsolete: true

Updated

5 years ago
Attachment #587955 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #587956 - Flags: review?(mrbkap) → review+
Gave this one last push to try to double-check: https://tbpl.mozilla.org/?tree=Try&rev=6bb5a094065b
Caught a bad parent, so I pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=c5294fcc3baf
Pushed this to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/46ae4bf4aaf4
http://hg.mozilla.org/integration/mozilla-inbound/rev/3ab1dcfb2218
Target Milestone: --- → mozilla12
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 ;)
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
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.
Target Milestone: mozilla12 → mozilla13
https://hg.mozilla.org/mozilla-central/rev/9dde014a4b3f
https://hg.mozilla.org/mozilla-central/rev/b89b725e10fb

Green PGO \o/
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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

Updated

5 years ago
Depends on: 726949
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
status-firefox13: --- → wontfix
Target Milestone: mozilla13 → mozilla14

Updated

5 years ago
Depends on: 785822

Updated

5 years ago
Depends on: 788966

Updated

5 years ago
Depends on: 790303
You need to log in before you can comment on or make changes to this bug.