Closed Bug 943512 Opened 6 years ago Closed 6 years ago

Stubify Promise.jsm so it can be used in the devtools debugger server

Categories

(Toolkit :: Async Tooling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bbenvie, Assigned: bbenvie)

References

Details

(Whiteboard: p=1)

Attachments

(1 file, 3 obsolete files)

As described in bug 881050 comment 20, in order for the devtools debugger server to use Promise.jsm it has to be loaded in the same compartment as the debugger server. This seems to only be doable by using loadSubScript. In order to satisfy the needs of the debugger server, Promise.jsm needs to be loadable via loadSubScript.
Blocks: 881050
Attached patch promise-stubify.patch (obsolete) — Splinter Review
This patch does the aforementioned stubification of Promise.jsm by introducing promise.js which can be loaded via loadSubScript.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Blocks: 943517
Attachment #8338700 - Flags: review?(dteller)
Hmmm, why would this cause a crash?
It appears to crash during the first rejection test, which involves running Cu.forceGC/forceCC/forceShrinkingGC. Is there any reason these would interact badly with loading Promise via loadSubScript?
Nope that's not it. The test still times out every time at the same spot.
Comment on attachment 8338700 [details] [diff] [review]
promise-stubify.patch

Cancelling review request until I fix this.
Attachment #8338700 - Flags: review?(dteller)
Replacing with needinfo. Do you have any idea why changing to loading this via loadSubScript would cause this test to fail?
Flags: needinfo?(dteller)
I have no clear idea, Just a few observations.
- looking at the stack, it seems that the cycle collector causes some JS code to be executed - I'm not sure that's safe;
- looking at the output, it seems that we're crashing during or shortly after the test that interacts with nsIFinalizationWitnessService - it makes sense that, if anything should crash, it's that.
Flags: needinfo?(dteller)
Panos, I think you said at the work week that wanted to take a look at what was happening with this crash?
Flags: needinfo?(past)
Unfortunately the logs are no longer available on tbpl and I can't get Firefox to crash locally with this patch. I may have botched rebasing though, as it has bitrotten a bit.
Flags: needinfo?(past)
Hmm it looks like the problem has been fixed.

https://tbpl.mozilla.org/?tree=Try&rev=7ba15efb6296
Attached patch promise-stubify.patch (obsolete) — Splinter Review
Rebased.
Attachment #8338700 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
I updated your patch to the latest version of the Promise module, and clarified the use of the JS file by renaming it to "Promise-backend.js" and keeping the documentation in the main Promise.jsm file.

If this sounds good to you, I think the change is ready to land.
Attachment #8386360 - Attachment is obsolete: true
Attachment #8389737 - Flags: review?(bbenvie)
I'll be updating the whiteboard of various Promise bugs with "p=" entries, indicating my take on the effort estimate and difficulty level of the bug. We're doing this in general for work in which the Firefox Desktop team is involved.
Whiteboard: p=1
Comment on attachment 8389737 [details] [diff] [review]
Updated patch

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

LGTM. I'll put r=paolo on this even though I'm giving the r+ here (since I'm just r+ing the diff between my patch and yours.
Attachment #8389737 - Flags: review?(bbenvie) → review+
Thanks! Landed on fx-team with r=paolo.

https://hg.mozilla.org/integration/fx-team/rev/1c3076666191
Backed out for numerous test failures. Please run this through Try before re-pushing.
https://hg.mozilla.org/integration/fx-team/rev/f9f13eefdc2d
Attached patch Fix for B2GSplinter Review
There were indeed failures on B2G, that looked like infrastructure failures on TBPL, but were caused by the fact that my patch didn't take into account the difference in the global scope of JSM modules between B2G and other platforms.

This is a tentative fix, let's see how it goes:

https://tbpl.mozilla.org/?tree=Try&rev=d9dc99a49c0b
Attachment #8389737 - Attachment is obsolete: true
The tryserver build looks reasonably green, with some known intermittent failures. Retrying:

https://hg.mozilla.org/integration/fx-team/rev/53e8cf0a814b
https://hg.mozilla.org/mozilla-central/rev/53e8cf0a814b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.