1195825 - Import promise.jsm via require(promise) in devtools codebase

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Followup after bug 1193313, where I just started removing unused require/import and got rid of promise.jsm aliases.
Now it would be great to replace all Cu.import(promise.jsm) by require(promise) in devtools codebase, that to lower the amount of Cu.import which looks weird for new contributors and also doesn't help understanding why we have various kind of promises whereas they are the same.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

As my previous refactoring patch, boring substitution everywhere...

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: side effect fixes v1

But unfortunately, this time, this refactoring introduces side effects.
For some reason, importing Promise symbol from require(promise)
is subtly different from Cu.import(Promise.jsm).
And it is actually subtly different!
require(promise) is an alias for Cu.import(Promise-backend.js)

It shouldn't change much, but it looks like it does.
I've looked at Promise-backend.js, added some debug,
but could identify any major difference between both way of importing it.

I've debugged browser/devtools/netmonitor/test/browser_net_resend.js quite a bit
and found that we have to wait a bit more after the CUSTOMREQUESTVIEW_POPULATED event
when we are using require(promise).
As if we were using sync promises or something that stress the event loop a little bit more.
But I couldn't really verify that as I don't know how to tell if we are sync or not?

Comment 3

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd45926e4bc

What do you think about this (see my two previous comments)?

This refactoring isn't as easy as the previous one, dark matter appears.
It may highlight something deeply wrong with require(promise),
but I'm wondering if I should pursue. Ok, I'm able to fix all the tests on try,
but it may introduce some races outside of just tests.

One very interesting point is that I identified which import/require was making browser/devtools/netmonitor/test/browser_net_resend.js to fail (its head.js promise import). And it looks like Cu.import(Promise.jsm) has the same behavior than DOM Promise. Or at least, if I replace the Cu.import with DOM Promise, the test doesn't fail!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hmm, that does seem quite surprising!  I can't see why the JSM vs. require(promise) would have different behavior, since the require() is just loading the same file as the JSM would have.

It still seems like a good idea to proceed with the conversion in general to me, assuming it's not a large burden to fix broken tests.

Maybe Paolo has an idea about the difference...?

Comment 5

[Alexandre Poirot [:ochameau]]

There is a difference. But I can't explain why it ends up modifying promise behavior/timings.

* Cu.import(Promise.jsm) use Promise-backend.js with JSM globals (with fully working `Components` global, and all various globals that exists in JSM but not sandboxes like `atob` and many others).
* require(promise) which is equivalent to require(promise-backend.js) use SDK Sandbox globals. So no `Components` global (need to use require(chrome)) and no other various globals.

Note that both environments have access to DOM Promise via `Promise` global.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> There is a difference. But I can't explain why it ends up modifying promise
> behavior/timings.
> 
> * Cu.import(Promise.jsm) use Promise-backend.js with JSM globals (with fully
> working `Components` global, and all various globals that exists in JSM but
> not sandboxes like `atob` and many others).
> * require(promise) which is equivalent to require(promise-backend.js) use
> SDK Sandbox globals. So no `Components` global (need to use require(chrome))
> and no other various globals.
> 
> Note that both environments have access to DOM Promise via `Promise` global.

Hmm, at the same time though, promise-backed.js does use require if it exists[1] to get chrome access, so it should be basically the same, right?

I did notice one direct use of `Components.stack`[2] which should be changed to `Components_.stack` to work in require() mode.  But, the code path doesn't seem like it would cause what you are seeing.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise-backend.js#43
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise-backend.js#191

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Hmm, at the same time though, promise-backed.js does use require if it
> exists[1] to get chrome access, so it should be basically the same, right?

Yes it should be the same Components object. I was more thinking about some other globals that may exists in JSM scope and not in SDK-Sandbox.

> I did notice one direct use of `Components.stack`[2] which should be changed
> to `Components_.stack` to work in require() mode.  But, the code path
> doesn't seem like it would cause what you are seeing.

That's the kind of thing that will be different. I think if we get in this code path, it is going to throw if loaded via SDK. I'll patch this tomorrow to see if it makes any difference to the failing tests.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> That's the kind of thing that will be different. I think if we get in this
> code path, it is going to throw if loaded via SDK. I'll patch this tomorrow
> to see if it makes any difference to the failing tests.

That doesn't change anything.
Also, I hacked Loader.jsm/Promise.jsm in order to map require(promise) to load Promise.jsm,
and it fixes the test. So it really relates to loading promise-backend.js directly.
I also benchmarked both promises and can't find any behavior difference in simple tests.

let t = [];
for(var i=0; i<10; i++) {
  let d = promise.defer();
  d.promise.then(()=>t.push("c"));
  t.push("a");
  d.resolve();
  t.push("b");}
}
console.log(t); // Both get a, b, c. Even under stress.

I dig a little within Promise-backend.js and it looks like DOMPromive.resolve, within scheduleWalperLoop is significantly "slower" when we do require(promise).
I verified that DOMPromise was really DOM Promises (that's the case).
I also tried using DOMPromise from a JSM, to see if there is any difference, but no. No difference.
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise-backend.js#746

What I see exactly is that, in the following code:
  DOMResolve.resolve().then(function f(){ ...walkerLoop... });
It ends up being slower to actuall call `f()` function when we use require(promise)... 

I also tried benchmarking DOM Promises within JSM scope versus Sandbox without seeing anything.

I don't know what's going on, really, so I'm going to stop wasting my time on this dark matter and pursue fixing the tests with sad workarounds. Hopefully, we can switch to DOM Promises completely, which don't behave badly.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: side effect fixes v2

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8650430 [details] [diff] [review]
patch v2

Here is the refactoring patch. Quite stupid patch. Sometimes I also cleanup imports by grouping Cu.import() vs require().
Also note that I'm not changing any variable name. If existing code used to name non-dom-promises `Promise` (with capital), I let it as-is.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8650431 [details] [diff] [review]
side effect fixes v2

I'm really sad to introduce such workarounds...
But it looks like tests are happy with that:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bb43bf4a51

Comment 13

[Alexandre Poirot [:ochameau]]

Also, while working on this, I've also discovered that some other module are also pulling yet another promise library: sdk/core/promise.

Comment 14

[Alexandre Poirot [:ochameau]]

Try run with more platforms and less patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=38ffbf6d9734
I opened bug 1196714 for the sdk/core/promise.

Comment 15

[Alexandre Poirot [:ochameau]]

Hum, I thought attachment 8650457 [details] [diff] [review] was related to bug 1137285, but it looks like it is yet another promise thing. But this fix actually makes sense.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1b3f5589d1f

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8650430 [details] [diff] [review]
patch v2

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

::: browser/devtools/webaudioeditor/panel.js
@@ +7,5 @@
>  
>  const { Cc, Ci, Cu, Cr } = require("chrome");
>  const EventEmitter = require("devtools/toolkit/event-emitter");
>  const { WebAudioFront } = require("devtools/server/actors/webaudio");
> +let Promise = require("promise");

Should we change to lowercase `promise` here to match others?

::: browser/devtools/webaudioeditor/test/head.js
@@ +17,5 @@
>  let { TargetFactory } = require("devtools/framework/target");
>  let { DebuggerServer } = require("devtools/server/main");
>  let { generateUUID } = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
> +let Promise = require("promise");

lowercase?

::: toolkit/devtools/async-utils.js
@@ +14,5 @@
>   */
>  
>  let {Cu} = require("chrome");
>  let {Task} = require("resource://gre/modules/Task.jsm");
> +let Promise = require("promise");

lowercase?

::: toolkit/devtools/shared/async-storage.js
@@ +40,5 @@
>   *
>   */
>  const {Cc, Ci, Cu, Cr} = require("chrome");
>  const {indexedDB} = require("sdk/indexed-db");
> +const Promise = require("promise");

lowercase?

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8650431 [details] [diff] [review]
side effect fixes v2

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

Not too happy to see setTimeout and friends...  Is there any other way?  A new event or something?

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Comment on attachment 8650430 [details] [diff] [review]
> ::: browser/devtools/webaudioeditor/panel.js
> @@ +7,5 @@
> >  
> >  const { Cc, Ci, Cu, Cr } = require("chrome");
> >  const EventEmitter = require("devtools/toolkit/event-emitter");
> >  const { WebAudioFront } = require("devtools/server/actors/webaudio");
> > +let Promise = require("promise");
> 
> Should we change to lowercase `promise` here to match others?

This is a pretty significant patch by itself.
If we want to do that, I would prefer doing it separatly.
Also, when I went ahead and did that, jwalker pushed back against it on gcli and just prefered switching to DOM Promises instead. So I think we should see with Jordan how does he fell for webaudio. For async, I think we should just do it, but in another patch.

Comment 19

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> Comment on attachment 8650431 [details] [diff] [review]
> side effect fixes v2
> 
> Review of attachment 8650431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not too happy to see setTimeout and friends...  Is there any other way?  A
> new event or something?

For the focus one, I don't know what we could be waiting for. This seems to be a race within platform focus code :/
I'll take a look at telemetry...

Comment 20

[:Paolo Amadini]

Thanks for working on this Alexandre!

Handling timing issues in Promise-based tests is always tricky, and when switching between Promise implementations most of the effort goes towards fixing tests that make assumptions about the relative timing of events. These issues happen with test code that clearly would fail intermittently if techniques like randomizing the order of events were applied.

Ideally these tests should be rewritten not to rely on event order anymore, but when me and Eddy were working on Promise refactorings we've also just introduced delays in some cases, when reverse engineering the test required a disproportionate effort.

The difference here is probably that each instance of the JavaScript Promise module has to spin its own main resolution loop by spawning a microtask with DOMPromise.resolve(). The "Cu.import" one and the "require" one are two different instances in memory, resulting in slightly different relative ordering of Promise resolutions.

The only blocker for switching away from Promise.jsm to DOM Promise is bug 989960, that impacts our ability to detect coding errors through our automated suites. That bug has been blocked for some time but is now actionable, we just need someone to take it.

Comment 21

[Alexandre Poirot [:ochameau]]

(In reply to :Paolo Amadini from comment #20)
> The difference here is probably that each instance of the JavaScript Promise
> module has to spin its own main resolution loop by spawning a microtask with
> DOMPromise.resolve(). The "Cu.import" one and the "require" one are two
> different instances in memory, resulting in slightly different relative
> ordering of Promise resolutions.

I don't see why any of the two would be any superior to the other one.
What I'm seeing here, is that the require() one seems to be *always* resolving faster.
As if sometimes it resolves faster on the event loop.
I don't see intermittent here, the behavior is always the same for each promise implementation.

But anyway, I think it is better to spend time to switch to DOM Promises and work on bug 989960 and others related to DOM one!

Hopefully, this patch will make it easier.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: side effect fixes v3

Got rid of setTimeout except for browser_net_resend.js and its focus race.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 8651774 [details] [diff] [review]
Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

Moving this patch from bug 1137285 and carrying r+ from it.

Comment 25

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d1304d746fa

Comment 26

[:Paolo Amadini]

(In reply to Alexandre Poirot [:ochameau] from comment #21)
> I don't see why any of the two would be any superior to the other one.
> What I'm seeing here, is that the require() one seems to be *always*
> resolving faster.
> As if sometimes it resolves faster on the event loop.

What happens is that each instance would try to invoke the handlers for all its resolved promises eagerly, before letting the other instance invoke its handlers. So if a Promise created by the require() instance is resolved first, it will prevent promises handlers for the Cu.import instance to run until all the resolved promises the require() instance knows about are handled.

Think about the order of Promise handler invocation, rather than order of Promise resolution. Yeah, it's very subtle :-)

> I don't see intermittent here, the behavior is always the same for each
> promise implementation.

Yes, the behavior is deterministic. This would be intermittent if we applied randomizing techniques to the order of events. These are sometimes used to detect test making assumptions like these in advance - there was someone working on it for our code base but I don't remember the details.

> But anyway, I think it is better to spend time to switch to DOM Promises and
> work on bug 989960 and others related to DOM one!

Definitely!

> Hopefully, this patch will make it easier.

Well, any change in Promise implementation would cause its own different changes in timings. If we fix the failing tests by implementing delays, we'll probably have to go through the process again when we switch to DOM Promise. That's fine with me by the way...

Comment 27

[Alexandre Poirot [:ochameau]]

(In reply to :Paolo Amadini from comment #26)
> > Hopefully, this patch will make it easier.
> 
> Well, any change in Promise implementation would cause its own different
> changes in timings. If we fix the failing tests by implementing delays,
> we'll probably have to go through the process again when we switch to DOM
> Promise. That's fine with me by the way...

So far, I've only seen require(promise) being different from Cu.import(Promise.jsm) *and* DOM Promise. Using Cu.import(Promise.jsm) or DOM Promise made no difference for the tests I had to fix in attachment 8651773 [details] [diff] [review]. So I'm hopeful regarding devtools codebase. Switching to DOM Promise might work as-is in many cases.

Comment 28

[Alexandre Poirot [:ochameau]]

Try reports a leak in tilt test. I don't have a debug build up'n ready.
I'll take a look at this tomorrow.

Comment 29

[Alexandre Poirot [:ochameau]]

Attachment: side effect fixes v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba8eeb21a2d
I wasn't able to fix the leak report in telemetry+tilt patch.
It took me quite a bit to first get rid of various exceptions
by waiting for correct events. But when you wait correctly
(which the current test doesn't!! We don't let time to tilt to operate),
a leak appears. I dig it a bit and the only object being "leaked"
is the "Tilt()" object instance. All other classes are freed correctly.
But given that Tilt() objects are in a weakmap indexed by chrome window,
we are most likely leaking the window somehow somewhere which then leak
these objects via the weakmap.
I can see Toolbox object being leaked also.

I'm quite reluctant to spend more time on this,
especially on tilt as we are turning e10s on and tilt doesn't work there...

Comment 30

[Alexandre Poirot [:ochameau]]

Comment on attachment 8652311 [details] [diff] [review]
side effect fixes v4

I'm expecting the new try to fail as it was still reporting leak in telemetry+tilt test. I would like to improve the test with this change but also disable it if that still fail on try.

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8652311 [details] [diff] [review]
side effect fixes v4

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

Okay, it seems better at least!  Thanks for working on it.  Agreed about tilt, probably best to not waste more time on it for not.

Comment 32

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #30)
> Comment on attachment 8652311 [details] [diff] [review]
> side effect fixes v4
> 
> I'm expecting the new try to fail as it was still reporting leak in
> telemetry+tilt test. I would like to improve the test with this change but
> also disable it if that still fail on try.

Hum, and now it seems to pass on try with these changes :x
Let's spawn some more runs...
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba8eeb21a2d
May be I have some other patch in my queue that introduces some other leaks.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/10e37cfc07d7
https://hg.mozilla.org/integration/fx-team/rev/952aa38e6460
https://hg.mozilla.org/integration/fx-team/rev/565f0b729ae3

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10e37cfc07d7
https://hg.mozilla.org/mozilla-central/rev/952aa38e6460
https://hg.mozilla.org/mozilla-central/rev/565f0b729ae3

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8651774 [details] [diff] [review]
Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

I missed this at review time, but the change in |_brieflyShowBoxModel| means that the promise returns by the function resolves after the box model has been hidden.
This, in turn, means that |_onNewSelection| calls the inspector-panel's |done| callback only after the box model has been hidden.
Which, in turn, means that the |inspector-updated| event is only fired after that's done.
To summarize, this means that mochitests that select nodes in the markup-view and wait for the inspector-updated event (to avoid pending requests on shutdown) have to wait 1 second before that happens.
There are some tests (like browser_markupview_navigation.js) that do a lot of node selections, and waiting for the inspector-updated event there makes the test timeout even locally on a fast machine (I'm trying to fix intermittent bug 1193733 which comes from a pending request).

While this is a problem for tests, it can also be one for the toolbox itself. There seems to be a couple of places where we wait for this event (variables-view and console) so this may unnecessarily delay certain user flows.

It doesn't seem useful to delay this event in order to wait for the brief highlighting to be over. Maybe we could not wait for the promise returned by |_brieflyShowBoxModel| and instead make sure tests that need this listen to |toolbox.once("node-highlight")| and |toolbox.once("node-unhighlight")|.

Alex, what do you think?

Comment 36

[Alexandre Poirot [:ochameau]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #35)
> Comment on attachment 8651774 [details] [diff] [review]
> 
> While this is a problem for tests, it can also be one for the toolbox
> itself. There seems to be a couple of places where we wait for this event
> (variables-view and console) so this may unnecessarily delay certain user
> flows.

Yep I didn't knew these was such scenario. I took care of not waiting for briefShowBoxModel to finish before calling showNode from within the markup view. That can obviously be an issue if we wait for inspector-updated for UI updates also.

> 
> It doesn't seem useful to delay this event in order to wait for the brief
> highlighting to be over. Maybe we could not wait for the promise returned by
> |_brieflyShowBoxModel| and instead make sure tests that need this listen to
> |toolbox.once("node-highlight")| and |toolbox.once("node-unhighlight")|.

In my experience, node-highlight is often not enough.
But may be a combination of one or multiple node-highlight plus inspector-updated can make it work?
One nice thing about inspector-updated is that it agreggates multiple pending actions whereas sometimes we have to listen for multiple node-highlight.
You can try reverting this change and just listen for node-highlight from this particular test.

> 
> Alex, what do you think?

What I think is that the whole story reach a barely managable complexity.
I'm starting to wonder if switching to react, with a bit more of sync, is going to help managing the pending requests issues?
I still have a bad feeling about all these events and having test-specific events.
Like here, you may fix this particular test, but not all the others that goes through this codepath.