920179 - Add resolve hook for IndexedDB constructors on non-window globals

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

I'm adding experimental methods on IndexedDB objects that rely on a pref being set before they're exposed. Unfortunately we eagerly define IndexedDB constructors for every global object at the moment (JetPack wants them so that they can be exposed through a sandbox) so there is no opportunity to set the pref in tests before the constructor objects are created. This basically means that testing these things is really hard. In mochitests I'll have to run everything in subframes, which is annoying. And it means that I cannot use xpcshell at all which is a pretty big problem (I use xpcshell for helgrind testing).

Attached is a resolve hook for indexedDB. This should make everything just work.

Comment 1

[Bobby Holley (:bholley)]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> I'm adding experimental methods on IndexedDB objects that rely on a pref
> being set before they're exposed. Unfortunately we eagerly define IndexedDB
> constructors for every global object at the moment

For every global object? I sure hope not. Do you mean for every DOM window?

> (JetPack wants them so that they can be exposed through a sandbox)

Can you elaborate on this?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> (In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> > I'm adding experimental methods on IndexedDB objects that rely on a pref
> > being set before they're exposed. Unfortunately we eagerly define IndexedDB
> > constructors for every global object at the moment
> 
> For every global object? I sure hope not. Do you mean for every DOM window?

Uh, so apparently this did indeed happen recently, at least for things that go through InitClassesWithNewWrappedGlobal. Andrea wrote the patches, and janv reviewed. I'm pretty unhappy about that, because it's something that I would have rminused for a number of reasons. Can you guys get review from an XPConnect peer next time?

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 809359 [details] [diff] [review]
Patch, v1

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

So this isn't really the right way to do it or the direction I want to be moving in. DOM constructors on non-DOMWindow globals is a more general problem that we're currently building out machinery for. But bent is a busy guy, and the current situation isn't his fault, so I'm not going to make him go fix it. r=bholley with comments addressed.

All of this stuff should use JS::HandleObject and company, per the recent style decision on dev-platform.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +957,5 @@
> +      if (constructor) {
> +        aObjp.set(aObj);
> +        return true;
> +      }
> +      return false;

I would interchange the order of these conditionals.

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +72,5 @@
> +    *_retval = ResolveWorkerClasses(cx, obj, id, flags, &objp);
> +    if (!*_retval) {
> +        *objpArg = nullptr;
> +        return NS_OK;
> +    }

Let's just null out objpArg on function entry and replace this with:

NS_ENSURE_TRUE(*_retval, NS_ERROR_FAILURE);

Here, above, and below.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +523,5 @@
>      //     (after bug 720580).
>      MOZ_ASSERT(js::GetObjectClass(global)->flags & JSCLASS_DOM_GLOBAL);
>  
>      // Init WebIDL binding constructors wanted on all XPConnect globals.
> +    // Additional bindings may be created lazily, see BackstagePass::NewResolve.

Please add a note here that people shouldn't add new constructors to this list without review from the XPConnect module owner.

Comment 4

[Jan Varga [:janv]]

(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > (In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> > > I'm adding experimental methods on IndexedDB objects that rely on a pref
> > > being set before they're exposed. Unfortunately we eagerly define IndexedDB
> > > constructors for every global object at the moment
> > 
> > For every global object? I sure hope not. Do you mean for every DOM window?
> 
> Uh, so apparently this did indeed happen recently, at least for things that
> go through InitClassesWithNewWrappedGlobal. Andrea wrote the patches, and
> janv reviewed. I'm pretty unhappy about that, because it's something that I
> would have rminused for a number of reasons. Can you guys get review from an
> XPConnect peer next time?

This all started in bug 890364 and bug 890382 and maybe sooner when TextEncoder/TextDecoder was added to InitiClassesWithNewWrappedGlobal.
I don't blame anyone, I just thought it was right way to do, sorry.

Comment 5

[Jan Varga [:janv]]

I'm touching InitClassesWithNewWrappedGlobal in bug 832883 too, but fortunately I'm adding a lazy getter for indexedDB global property, I'll requeest a review from you once I merge it with this resolve hook patch.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jan Varga [:janv] from comment #4)
> This all started in bug 890364 and bug 890382 and maybe sooner when
> TextEncoder/TextDecoder was added to InitiClassesWithNewWrappedGlobal.
> I don't blame anyone, I just thought it was right way to do, sorry.

Yeah, it looks like bz suggested this, at which point I don't blame you. ;-)

I filed bug 920553 to sort all this out.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d49b0f47b05a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows crashes in |make package|.
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b202186812

Comment 9

[Ryan VanderMeulen [:RyanVM]]

And checktest failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=28417998&tree=Mozilla-Inbound

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wow, my first MSVC optimizer bug!

Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/33a6ceca176d

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Interdiff

Comment 12

[(no longer active)]

(In reply to comment #10)
> Wow, my first MSVC optimizer bug!

Welcome to the club!  There's a yearly membership fee of $100, you can send the check to me.  ;-)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33a6ceca176d