Closed Bug 1388054 Opened 3 years ago Closed 1 year ago

Stop using Promise.jsm from devtools/shared/defer

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

Removing dependency on Promise.jsm from devtools/shared/defer appears to be more challenging than the other modules.
It makes two tests fail because of changes to the stack behavior when Promises are involved:
TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_protocol_stack.js | run_test/onConnect/< - [run_test/onConnect/< : 86] Incomplete stack - false == true
TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_executeSoon.js |  - Incomplete stack - false == true

For test_protocol_stack.js, it is simple, there is just a change is the way the stacks are stringified.

For test_executeSoon.js it is another story as it seems to highlight a loss of information in the stacks.
http://searchfox.org/mozilla-central/source/devtools/shared/tests/unit/test_executeSoon.js#28-40
  yield waitForTick();
  let stack = Components.stack;
  ...
We expect to find the call to waitForTick in this stack.

Here the the stack with current tip:
@/mnt/desktop/gecko/obj-firefox-artifact/_tests/xpcshell/devtools/shared/tests/unit/test_executeSoon.js:29:17
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
DevToolsUtils.executeSoon*exports.executeSoon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:57:19
waitForTick@/mnt/desktop/gecko/obj-firefox-artifact/_tests/xpcshell/devtools/shared/tests/unit/test_executeSoon.js:47:3
@/mnt/desktop/gecko/obj-firefox-artifact/_tests/xpcshell/devtools/shared/tests/unit/test_executeSoon.js:28:9
TaskImpl_run@resource://gre/modules/Task.jsm:331:42
TaskImpl@resource://gre/modules/Task.jsm:280:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
_run_next_test@/mnt/desktop/gecko/testing/xpcshell/head.js:1488:9
run@/mnt/desktop/gecko/testing/xpcshell/head.js:701:9
_do_main@/mnt/desktop/gecko/testing/xpcshell/head.js:221:3
_execute_test@/mnt/desktop/gecko/testing/xpcshell/head.js:544:5
@-e:1:1

You see everything, the executeSoon, waitForTick and Promise.jsm internals!

But here is what you get if you make defer use DOM Promise:
@/mnt/desktop/gecko/obj-firefox-artifact/_tests/xpcshell/devtools/shared/tests/unit/test_executeSoon.js:29:17
_do_main@/mnt/desktop/gecko/testing/xpcshell/head.js:221:3
_execute_test@/mnt/desktop/gecko/testing/xpcshell/head.js:544:5
@-e:1:1

It looks like async stack don't work at all across DOM Promises...

It is unclear what is the expected behavior here regarding DOM Promise,
but it seems to clearly differ from Promise.jsm.

* It looks like stack are correct when throwing and looking at exception stacks:
  
  new Promise(function foo(done) {throw new Error("err")}).then(null, function bar(e) {console.log(e.stack);});

  - foo@debugger eval code:1:39
  - @debugger eval code:1:1

  The important point here is that we see "foo" function in the stack!

* But the is a loss of information when looking at current stack from promise handlers:

  new Promise(function foo() {throw new Error("err")}).then(null,function bar(e) {console.log(new Error().stack);});

  - bar@debugger eval code:1:93
  - promise callback*@debugger eval code:1:1

  new Promise(function foo(done) {done()}).then(function bar(e) {console.log(new Error().stack);});

  - bar@debugger eval code:1:76
  - promise callback*@debugger eval code:1:1

There is this strange "promise callback" item, but we don't see "foo" in the stack.


I ran all these tests with javascript.options.asyncstack set to true.



I tried to look at Promise.cpp and it is unclear how async stack are supported, if they are. It looks like it correctly register instanciation, and settlement for PromiseDebugging:
http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#3178-3185
http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#1290-1298
But at first look, I don't see anything to help supporting async stack.
It looks like it should involve "JS::AutoSetAsyncStackForNewCalls" in order to help supporting that, but I can't see any usage of this. Or is it using AsyncFunction.cpp, which uses AutoSetAsyncStackForNewCalls...??
This change also results in this mochitest failure:
devtools/client/framework/test/browser_target_events.js | A promise chain failed to handle a rejection: unsafe CPOW usage forbidden - stack: handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:83:7
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:137:11
onLocationChange@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:789:9

This may relate to bug 1386299.
Before event-emitter was in a JSM, it may have still be able to use CPOW,
whereas it is clear that DevTools modules can't since bug 1355994 landed.

This exception comes from this line:
http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#789
which looks very outdated, broken and useless:
1) Navigate should come with an object as first argument, not a window object.
Like this one:
  http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#534-538
2) It looks like there is no code using navRequest/navPayload:
  http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#552-558
3) It looks completely redundant with the tabNavigated event that the server should send:
  http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#561
  It looks like setupListeners could possibly be completely redundant with setupRemoteListeners...

It may be worth to also fix that in a dedicated bug.
Priority: -- → P3
This may be a dupe of bug 1438121, since stacks captured by Error also use SavedFrame.

See bug 1438121 comment 16 for a summary of the cause.
Product: Firefox → DevTools
I think that all the roadblocks have been adressed.
Stacks have been fixed by Jim on bug 1438121 and EventEmitter have been fixed in bug 1386299.
Let's see what try says today.
Try is green, we should be able to proceed with this patch, finally!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a600b75a946ec86cb7a42ca9de3cda0589a0f5f
Comment on attachment 8986433 [details]
Bug 1388054 - Stop using Promise.jsm from devtools/shared/defer.

https://reviewboard.mozilla.org/r/251794/#review258244

Great, thanks to everyone who helped here! :)
Attachment #8986433 - Flags: review?(jryans) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0605328631fd
Stop using Promise.jsm from devtools/shared/defer. r=jryans
Ah. Right. Shader editor may be really run only on osx because it needs webgl to work on CI.
Now, it is unclear why it makes this test to fail except just being a race.
It should not be related to panel's document promises as defer is using a system Promise...
Flags: needinfo?(poirot.alex)
Depends on: 1470086
Depends on: 1455421

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2078b58fe83a
Stop using Promise.jsm from devtools/shared/defer. r=jryans

Keywords: checkin-needed

Panos, be careful about the landmine you are walking on here.

This bug was blocked on bug 1455421 and the issues around Zombie promises.
This zombie promises issue was only highlighted by the now-removed shader editor and webaudio panels.
Only these panels were making try to fail. But that particular issue may impact any panel.
A zombie Promise is a DOM Promise coming from the panels (inspector, console, ...)
which becomes zombie when the related panel's document is destroyed.
These promises may come from a call doing new Promise(), but also from async function(){}, which returns a Promise.
When the panel is destroyed, these promise may be stall and never resolve, even if some chrome code call their resolve or reject handler!
This leads to half destroyed blank toolbox, which can't be toggled on again.

I was confused about why this particular patch was trigerring an issue similar to the zombie promises,
as devtools/shared/defer should never be loaded as a Browser Loader module and so, never use DOM Promises coming from the panel's document. So I don't know how much trouble we can get out of it...

The very latest action item to unblock us here is to work on bug 1529621.
If we get rid of all async destruction code, we can then safely get rid of Promise.jsm.
Until then, we can't fully switch to DOM Promises.

Flags: needinfo?(past)

Thanks for the heads up, I appreciate the background information. I've read through the referenced bugs and it sounds like the plan you have laid out is the right one.

I'm ambivalent about what specifically to do about this patch. Since all the breakage that has been discovered in the past (comment 8, bug 1402779, bug 1455226) has either been in panels that we have removed or panels that we have fixed, I don't see a way to verify whether this patch will create breakage at all. Not having an automated test to catch this is clearly a problem, but I would even settle for a manual test. So far all my attempts to reproduce the breakage have been unsuccessful, and I've tried many panels that have been mentioned in these bugs (console, inspector, memory, perf, etc.) in various configurations (docked, undocked, etc.).

Based on the above, I'm leaning towards letting this patch get merged to m-c and see if we get any bug reports to back it out. Would you rather have it backed out immediately instead?

Flags: needinfo?(past) → needinfo?(poirot.alex)
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Assignee: nobody → poirot.alex

(In reply to Panos Astithas (he/him) [:past] (please ni?) from comment #13)

Based on the above, I'm leaning towards letting this patch get merged to m-c and see if we get any bug reports to back it out. Would you rather have it backed out immediately instead?

In theory, this patch shouldn't trigger zombie promises.
So it is really unclear why it triggered zombie promises-like failures back in the days. May be it was only a timing issue?
I agree that it should be ok to keep it landed. My warning was especially about doing any progress on removing the last bits of Promise.jsm, which, for sure could trigger the zombie promises.

One intermediate step you could do if you are looking forward removing Promise.jsm is to replace them by the special "always alive" DOM Promise.
So instead of removing promise = require("promise") and replace promise by Promise,
which would switch from Promise.jsm to DOM Promise,
you could do replace the import by promise = require("Promise") and replace promise by Promise.
This would switch from Promise.jsm to "always alive" DOM Promise coming from a special chrome context.
This could still slightly change the timings and highlight an already existing zombie promise. But by itself it doesn't introduce new case of potential zombie promises. We typically have zombie promises today when we are using async function in any destroy codepath of the panels.

Flags: needinfo?(poirot.alex)
You need to log in before you can comment on or make changes to this bug.