585786 - Get rid of slimwrapper checks in quickstubs when we can

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

e.g. once we only have slimwrappers for DOM objects?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is biting us for things like ownerDocument, because our wrappers have morphed.

Peter, Blake, why do we really need this check?

Comment 2

[Peter Van der Beken [:peterv]]

I'm not a fan of moving all the other checks into that function (NeedsSOW, outerize, ...?).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, sure.  The question is, which of those checks do we need here?

Specifically, given that we know this is a same-compartment access to an object that has a wrapper cache, what additional checks do we actually need to do?

I guess there's at least the QI stuff that NativeInterface2JSObject does....  Is that relevant for quickstubs?

Maybe I should turn this around.  Are there ways to tell at compile time which quickstubs don't need this jazz?  I mean.. .ownerDocument should be able to return whatever is in the document's wrapper cache, period, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note: we can't end up with slimwrappers for Document objects at the moment, because chrome pokes them from all sorts of events so we morph when we try to security-wrap....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Do fast-unwrapping even for non-slim wrappers as long as they're cached.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 493177 [details] [diff] [review]
Do fast-unwrapping even for non-slim wrappers as long as they're cached.

This leaves the IsProxy() and NeedsSOW() cases slow, but the latter is very rare in web script and the former is just slow; not much to do about it.

I did run the numbers, and they look good.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 493177 [details] [diff] [review]
Do fast-unwrapping even for non-slim wrappers as long as they're cached.

>+    NS_ABORT_IF_FALSE(IS_WN_WRAPPER(cache->GetWrapper()),
>+                      "Must have XPCWrappedNative wrapper");
>+    return
>+        !cache->IsProxy() &&

Won't we abort if cache->IsProxy()?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Um... yes.  I'll fix the abort to check for IsProxy() too.

Comment 9

[Johnny Stenback (:jst)]

Not blocking, but I'll gladly approve a reviewed patch.

Comment 10

[Peter Van der Beken [:peterv]]

Don't we also need to special-case the location wrapper? (look for xpc::WrapperFactory::IsLocationObject in XPCConvert::NativeInterface2JSObject)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Location objects don't have a wrapper cache.  I'd be happy to add asserts to this effect in NativeInterface2JSObject, which would catch it if that ever stopped being true.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 493177 [details] [diff] [review]
Do fast-unwrapping even for non-slim wrappers as long as they're cached.

Good point :-). Doesn't look like there's an easy way to make sure this stays in sync with XPCConvert::NativeInterface2JSObject :-(.

r+ with the abort changed. Also might want to change the name of xpc_GetCachedSlimWrapper (xpc_FastGetCachedWrapper? not sure...).

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Doesn't look like there's an easy way to make sure this stays in sync with
> XPCConvert::NativeInterface2JSObject :-(.

That's true, though I'm still happy to add that assert from comment 11.

And yeah, good point about the name.  I like xpc_FastGetCachedWrapper.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: With review comments addressed

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/9e68a5126961