Closed Bug 920179 Opened 6 years ago Closed 6 years ago

Add resolve hook for IndexedDB constructors on non-window globals

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(2 files)

Attached patch Patch, v1Splinter Review
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.
Attachment #809359 - Flags: review?(bobbyholley+bmo)
(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?
(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 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.
Attachment #809359 - Flags: review?(bobbyholley+bmo) → review+
(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.
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.
(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.
(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.  ;-)
https://hg.mozilla.org/mozilla-central/rev/33a6ceca176d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.