Closed Bug 943512 Opened 6 years ago Closed 6 years ago

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


(Toolkit :: Async Tooling, defect, P2)






(Reporter: bbenvie, Assigned: bbenvie)



(Whiteboard: p=1)


(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
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]

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.
Attached patch promise-stubify.patch (obsolete) — Splinter Review
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.
Backed out for numerous test failures. Please run this through Try before re-pushing.
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:
Attachment #8389737 - Attachment is obsolete: true
The tryserver build looks reasonably green, with some known intermittent failures. Retrying:
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.