1126350 - Keep destroying the toolbox even if a panel fails

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

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

While investigating bug 1118974, I noticed that the toolbox destruction path will abort early if any one of the panels fails.

We should rewrite this to continue destruction of all panels and the toolbox, even if one part fails, and should just log a message when there is an error.

Comment 1

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

Attachment: MozReview Request: bz://1126350/jryans

/r/3177 - Bug 1126350 - Settle all toolbox destruction before final cleanup. r=pbrosset

Pull down this commit:

hg pull review -r c1d921d17fb59f805ff179f26eac7e7a4e20839d

Comment 2

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=265bb729311a

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8557255 [details]
MozReview Request: bz://1126350/jryans

https://reviewboard.mozilla.org/r/3175/#review2657

Ship It!

::: toolkit/devtools/DevToolsUtils.js
(Diff revision 1)
> + * This differs from Promise.all, which will reject immediately after the first

I was surprised this was the case, but yeah, it does look like we need a helper to wait for all promises to resolve or reject.

::: toolkit/devtools/DevToolsUtils.js
(Diff revision 1)
> +  return new Promise((resolve, reject) => {

I'm all for using DOM promises, but have only seen promise.jsm promises in our code so far (also there's a require("promise") in this file).
What's our approach here? Slowly start to insert DOM promises step by step, or use promise.jsm only and then do a big-bang migration?

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8557255 [details]
MozReview Request: bz://1126350/jryans

https://reviewboard.mozilla.org/r/3175/#review2659

Ship It!

Comment 5

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

https://reviewboard.mozilla.org/r/3175/#review2675

> I'm all for using DOM promises, but have only seen promise.jsm promises in our code so far (also there's a require("promise") in this file).
> What's our approach here? Slowly start to insert DOM promises step by step, or use promise.jsm only and then do a big-bang migration?

There are a few [usages][1] already, but mostly in tests.

Personally, it seems fine to me to make use of them piecemeal, but it's mostly a style point I would think.  I'll email the list to ask the team about it.

In this case, it appeared because I copied Promise.jsm's implemtation of |all| as a starting point, which itself uses this DOM promise.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=%22new%20Promise%22%20path%3Adevtools&case=true&=mozilla-central

Comment 6

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

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> Personally, it seems fine to me to make use of them piecemeal, but it's
> mostly a style point I would think.  I'll email the list to ask the team
> about it.

Consensus was the team prefers Promise.jsm for now because of the extra debugging features.  I'll switch to that.

Comment 7

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

Updated try with Promise.jsm:

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

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/c70ba1f08a7f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c70ba1f08a7f

Comment 11

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

Attachment: MozReview Request: Bug 1126350 - Settle all toolbox destruction before final cleanup. r=pbrosset