I think everything is in the title. I faced some races around toolboxPromise from webide.js: http://mxr.mozilla.org/mozilla-central/source/devtools/client/webide/content/webide.js#986 It ends up being nullified late in the process, after toolbox.destroy()'s promise is resolved. That end up possibly introducing races in tests as (yield toolbox.destroy()) won't wait enough if you query toolboxPromise right after. My patch listen to toolbox's destroyed event in order to cleanup the promise. That ensures doing it before the promise resolves. I've kept the message listener for toolbox-close as it may cover some other codepath...(I haven't verified that)

Here is the patch I was talking about earlier today...

Comment on attachment 8668610 [details] [diff] [review] patch v1 Review of attachment 8668610 [details] [diff] [review]: ----------------------------------------------------------------- I think it's okay, but could you describe the precise situation that used to fail and is now fixed? Is it lucid dream tests only? Or something a user can perform? Also, what do you think of `destroyToolbox` in WebIDE? There some place we yield on it, which means we block on `toolbox.destroy`. Should we? I wish we had an exhaustive test of all the ways to close a toolbox, to ensure it **always** closes, no matter what... ::: devtools/client/webide/content/webide.js @@ +1028,5 @@ > } > this.toolboxPromise = AppManager.getTarget().then((target) => { > return this._showToolbox(target); > }, console.error); > + // In parrallel of toolbox-close event received from CustomHost, Nit: parallel

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > > I think it's okay, but could you describe the precise situation that used to > fail and is now fixed? Is it lucid dream tests only? Or something a user > can perform? So far, I've only seen that while running luciddream, but that's not impossible it can introduce broken toolbox if we try to reopen one *just after* another one is closed. I imagine you have to stress your computer quite a bit in order to get into this codepath. > > Also, what do you think of `destroyToolbox` in WebIDE? There some place we > yield on it, which means we block on `toolbox.destroy`. Should we? Yes waiting on toolbox.destroy is a good thing, it ensures various bits are toggle correctly. > I wish we had an exhaustive test of all the ways to close a toolbox, to > ensure it **always** closes, no matter what... Here is a test that covers just that: In automated tests, we expect to be able to reopen a (webide) toolbox once toolbox.destroy()'s promise is resolved.

Comment on attachment 8672680 [details] [diff] [review] patch v2 Review of attachment 8672680 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding a test! ::: devtools/client/webide/content/webide.js @@ +972,5 @@ > return this._showToolbox(target); > }, console.error); > + // In parallel of toolbox-close event received from CustomHost, > + // also listen to toolbox destroyed event as it fires earlier. > + // That, to destroy the toolbox on WebIDE side before Nit: Replace "That," with something like "The goal is" or "We want"

Delaying this patch, as I think it introduced some other issues (that may already exist with the existing codepath). It looks like, when we create two toolboxes in a very short period of time, the `destroyed` (same of toolbox-close) listener mess up and do not remove the toolbox as toolboxPromise is null. That's the behavior I've reproduced on 1215049. It also looks like we may also remove the newly created toolbox, but I haven't reproduced this.

Finally, there was various issues with my previous patch. I also had to do various other fixes everywhere. I tried to cover everything with tests but it's not always possible. But I think with all theses patches, we are going to be in a much much better state. (bug 1215049, bug 1216550, bug 1216554, bug 1216555, bug 1210429) I played with all these patches and wasn't able to reproduce a broken toolbox, even while clickling furiously in all apps/tabs/wrench-icon!! A try run with all of them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eacee2dae9be I wouldn't be surprised if tests are broken given the changes I made in bug 1216554.

Comment on attachment 8676274 [details] [diff] [review] patch v3 Review of attachment 8676274 [details] [diff] [review]: ----------------------------------------------------------------- I gave this a try with all the other patches applied and it seems to work quite well! :D Great work, these issues are always frustrating. Mostly I want some clarification / more comments, as it's hard to know what's happening when. It does seem simpler than the previous version though! :) ::: devtools/client/webide/content/webide.js @@ +926,5 @@ > + * * Close button inside the toolbox > + * * Toggle toolbox wrench in WebIDE > + * * Disconnect the current runtime gracefully > + * * Yank cord out of device > + * * close or crash the app/tab Nit: Use "Close" with capital C to match the previous lines @@ +936,5 @@ > + // If that's the currently displayed toolbox, > + // we also cleanup other stuff than just the toolbox. > + // Otherwise, this is a race, we are still destroying a previous toolbox, > + // while another one has been loaded, and we just remove the toolbox iframe. > + if (!this.toolboxPromise || this.toolboxPromise === promise) { I can't follow the `!this.toolboxPromise` part... why do we need these steps if `toolboxPromise` is null? If it's correct, please add more comments. @@ +947,5 @@ > + document.querySelector("#action-button-debug").removeAttribute("active"); > + } > + // We have to destroy the iframe, otherwise, the keybindings of webide don't work > + // properly anymore. > + iframe.remove(); How do we know this is safe to do in all cases, while the other steps are not? Sorry, it's hard to imagine all possible cases... @@ +955,5 @@ > // Only have a live toolbox if |this.toolboxPromise| exists > if (this.toolboxPromise) { > let toolboxPromise = this.toolboxPromise; > this.toolboxPromise = null; > + toolboxPromise.then(toolbox => toolbox.destroy()); Should we return this value to block the caller on destroy? Or did you change that on purpose?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > Comment on attachment 8676274 [details] [diff] [review] > @@ +936,5 @@ > > + // If that's the currently displayed toolbox, > > + // we also cleanup other stuff than just the toolbox. > > + // Otherwise, this is a race, we are still destroying a previous toolbox, > > + // while another one has been loaded, and we just remove the toolbox iframe. > > + if (!this.toolboxPromise || this.toolboxPromise === promise) { > > I can't follow the `!this.toolboxPromise` part... why do we need these > steps if `toolboxPromise` is null? > > If it's correct, please add more comments. It's very correct ;) I tried to update the comment to clarify the whole story. toolboxPromise==null is for when it is the last toolbox opened, closed by an explicit WebIDE order, when it calls UI.destroyToolbox(). The promise is nullified immediately whe we call destroyToolbox. > > @@ +947,5 @@ > > + document.querySelector("#action-button-debug").removeAttribute("active"); > > + } > > + // We have to destroy the iframe, otherwise, the keybindings of webide don't work > > + // properly anymore. > > + iframe.remove(); > > How do we know this is safe to do in all cases, while the other steps are > not? This is the very precise trick that allows to get rid of all toolboxes no matter what. If we put any condition around iframe removal, we are surely going to keep a dead one alive. It is safe as, we were already doing it before, we were just *not* doing it in same racy cases. Also, this is done when `destroyed` event is fired, which is when toolbox.destroy()'s promise is resolved. So it should be safe to get rid of the toolbox at this point. > @@ +955,5 @@ > > // Only have a live toolbox if |this.toolboxPromise| exists > > if (this.toolboxPromise) { > > let toolboxPromise = this.toolboxPromise; > > this.toolboxPromise = null; > > + toolboxPromise.then(toolbox => toolbox.destroy()); > > Should we return this value to block the caller on destroy? > > Or did you change that on purpose? No. I just wanted to get rid of the unecessary console.error as Promises by itself shows unhandeld rejections. I updated to return the promise.

Comment on attachment 8676804 [details] [diff] [review] patch v4 Review of attachment 8676804 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the extra comments!

https://hg.mozilla.org/integration/fx-team/rev/43d38fbbc50e84d9b9d9217e9346bf354918a91e Bug 1210429 - Fix race in webide toolbox destruction. r=jryans