Closed Bug 585786 Opened 10 years ago Closed 9 years ago

Get rid of slimwrapper checks in quickstubs when we can

Categories

(Core :: XPConnect, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- -

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

e.g. once we only have slimwrappers for DOM objects?
This is biting us for things like ownerDocument, because our wrappers have morphed.

Peter, Blake, why do we really need this check?
Blocks: 614164
blocking2.0: --- → ?
I'm not a fan of moving all the other checks into that function (NeedsSOW, outerize, ...?).
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?
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 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.
Attachment #493177 - Flags: review?(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()?
Um... yes.  I'll fix the abort to check for IsProxy() too.
Not blocking, but I'll gladly approve a reviewed patch.
Assignee: nobody → bzbarsky
blocking2.0: ? → -
Priority: -- → P1
Whiteboard: [need review]
Don't we also need to special-case the location wrapper? (look for xpc::WrapperFactory::IsLocationObject in XPCConvert::NativeInterface2JSObject)
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 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...).
Attachment #493177 - Flags: review?(peterv) → review+
> 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.
Attachment #493177 - Attachment is obsolete: true
Attachment #495538 - Flags: approval2.0?
Whiteboard: [need review] → [need approval]
Whiteboard: [need approval] → [need gk2 ship]
Attachment #495538 - Flags: approval2.0? → approval2.0-
Pushed http://hg.mozilla.org/mozilla-central/rev/9e68a5126961
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need gk2 ship]
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.