1053311 - Refactor Promise.jsm so it can be used in workers

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Eddy Bruel [:ejpbruel]]

I've ran into a problem with bug 1035206 where none of the builtin APIs that are normally available to workers, such as timeouts, (native) promises, or XHR, work properly in the presence of nested event loops.

To work around this, we've decided on a solution where we are going to make timers behave properly in the presence of nested event loops in workers (see bug 757133), and then make Promise.jsm use those timers instead of the thread manager in workers (recall that the Services object isn't available in workers).

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Patch

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8472426 [details] [diff] [review]
Patch

eh?

Comment 3

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8472426 [details] [diff] [review]
Patch

Sorry about that, hg blame claimed ryanvm wrote most of Promise-backend.js, so I assigned it to you without thinking :-D

Assigning to past since I can't think of a better candidate. This cannot land anyway until the platform stuff lands, so there's no big rush.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8472426 [details] [diff] [review]
Patch

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

Looks good to me, but Paolo is a better reviewer for the promise changes and Mossop should probably look at the Services change.

::: toolkit/modules/Promise-backend.js
@@ +8,5 @@
>  
>  /**
>   * This implementation file is imported by the Promise.jsm module, and as a
> + * special case by the worker loader. Since we do not have access to Components
> + * in workers, we cannot use of Cu.import. We therefore require this file

Typo: "we cannot use Cu.import"

Comment 5

[:Paolo Amadini]

Comment on attachment 8472426 [details] [diff] [review]
Patch

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

(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> This cannot land anyway until the platform stuff lands, so there's no big rush.

Cool. I'd appreciate if you could rebase this patch on top bug 1033406, which I expect will land within a few days.

::: toolkit/modules/Promise-backend.js
@@ +29,5 @@
>  
> +// If we are being required as an SDK module by the worker loader, isWorker is
> +// defined as true. Otherwise, isWorker is undefined.
> +if (!this.isWorker) {
> +  this.isWorker = false; // Make sure isWorker is defined

Maybe you can just set this.isWorker to false in Promise.jsm?

Then you can just check "isWorker" instead of "this.isWorker" here, like you do in other cases.

::: toolkit/modules/Services.jsm
@@ +78,5 @@
>    ["telemetry", "@mozilla.org/base/telemetry;1", "nsITelemetry"],
>    ["tm", "@mozilla.org/thread-manager;1", "nsIThreadManager"],
>    ["urlFormatter", "@mozilla.org/toolkit/URLFormatterService;1", "nsIURLFormatter"],
>    ["vc", "@mozilla.org/xpcom/version-comparator;1", "nsIVersionComparator"],
> +  ["witness", "@mozilla.org/toolkit/finalizationwitness;1", "nsIFinalizationWitnessService"],

I'd keep the lazy getter in Promise.jsm, it doesn't seem to be used elsewhere. You can place the lazy getter within an isWorker check, or make your own implementation if you don't want to import XPCOMUtils.

I get you have tests elsewhere that indirectly cover the usage of the Promise backend as an SDK module, right?

Comment 6

[Eddy Bruel [:ejpbruel]]

I think this somehow landed as part of another patch/bug, but I don't actually know which one :-S

In any case, this seems to work now, since worker debugging just landed.