1117067 - You can't execute any action with WebIDE if you disconnect and reconnect a B2G device.

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Johan Lorenzo [:jlorenzo]]

Nightly build ID: 20150101030214
Executed on Arch Linux.
e10s is deactivated.

STR
1. Connect a B2G device
2. Open an app (like E-Mail)
3. Execute an action (like picking an element with the inspector) => the action gets executed
4. Disconnect the device with the disconnect button
5. Reconnect it
6. If the app is not reopen, do it
7. Execute the same action

Actual result
Nothing happens, no error is reported. The inspector page is blank. I also tried to execute something within the console, I am able to type something but pressing enter doesn't execute the code (it adds a new line instead).

Restarting WebIDE cleans up the state, and everything works well (until the next disconnection of the device)

Additional info
As a member of the B2G automation team, I disconnect my device from WebIDE multiple times a day. Closing and reopening WebIDE each time is a pain point.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Do you have the same issue on your Mac, Martijn?

Comment 2

[Martijn Wargers (dead)]

Yes, I can reproduce.
Thanks for filing, I had issues, where webIDE randomly seemed to stop working. I think that's the same issue.

Comment 3

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

Johan, could you check if there are any errors in the Browser Console?

This will help find the cause of the issue.

Comment 4

[Johan Lorenzo [:jlorenzo]]

The browser console becomes blank once the device is disconnected. Nothing is displayed and you can't use it (like by typing 'console.log("foo")' and pressing enter, nothing seems to happen).

Where can I get additional information?

Comment 5

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

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #4)
> The browser console becomes blank once the device is disconnected. Nothing
> is displayed and you can't use it (like by typing 'console.log("foo")' and
> pressing enter, nothing seems to happen).
> 
> Where can I get additional information?

Hmm, really?  I've not heard of this issue before...

Just to be sure, we're both talking about the tool reached from Tools -> Web Developer -> Browser Console[1], correct?

To be sure I understand your steps, you open Browser Console first, and then following the steps in comment 0, and after step 4 (disconnect device), the Browser Console window suddenly goes blank?

[1]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Console

Comment 6

[Johan Lorenzo [:jlorenzo]]

Attachment: Screencast

You're right, I thought the Browser console was the one in WebIDE. Here's the error I get in the (actual) browser console:

> Front.prototype.send threw an exception: TypeError: this.conn._transport is null
> Stack: Front<.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1152:9
> Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
> this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
> Line: 1152, column: 8

I made a screencast (see attachment), to make STR more clear.

Comment 7

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

Attachment: MozReview Request: bz://1117067/jryans

/r/3169 - Bug 1117067 - Destroy toolbox before disconnect. r=ochameau

Pull down this commit:

hg pull review -r 5b49a9202862a1e8a2e7a67217dda27da764daa3

Comment 8

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

I could have sworn I made this change before...  Maybe I did it in App Manager. :)

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

Comment 9

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/3169/#review2689

::: browser/devtools/webide/content/webide.js
(Diff revision 1)
> +      yield UI.destroyToolbox();

destroyToolbox implementation looks misleading.
We wait for toolboxPromise completion, but not for toolbox.destroy(), also it looks like we meant to display toolbox.destroy() exception.
But as far as I understand promises, the first callback to then doesn't call the second one if there is an exception.
(I'm refering to the second argument of then(), console.error)
return this.toolboxPromise.then(tb => {this.toolboxPromise = null; return tb.destroy()})
                          .then(null, console.error);

1/ a comment about why we destroy the toolbox before disconnecting would be useful
2/ in order to really fix all issues, especially on slow hardware, I imagine we want to wait for full toolbox destruction before disconnecting, so that we would need some tweaks in destroyToolbox

Comment 10

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

https://reviewboard.mozilla.org/r/3169/#review2705

> destroyToolbox implementation looks misleading.
> We wait for toolboxPromise completion, but not for toolbox.destroy(), also it looks like we meant to display toolbox.destroy() exception.
> But as far as I understand promises, the first callback to then doesn't call the second one if there is an exception.
> (I'm refering to the second argument of then(), console.error)
> return this.toolboxPromise.then(tb => {this.toolboxPromise = null; return tb.destroy()})
>                           .then(null, console.error);
> 
> 1/ a comment about why we destroy the toolbox before disconnecting would be useful
> 2/ in order to really fix all issues, especially on slow hardware, I imagine we want to wait for full toolbox destruction before disconnecting, so that we would need some tweaks in destroyToolbox

Yes, you're right that we're not waiting correctly for things to finish.

I was actually down this rabbit hole for a different bug the other day...

Anyway, I'll have you review an additional patch to clean this up.

Comment 11

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

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

/r/3169 - Bug 1117067 - Destroy toolbox before disconnect. r=ochameau
/r/3301 - Bug 1117067 - Harden toolbox cleanup paths, async promises for WebIDE tests. r=ochameau

Pull down these commits:

hg pull review -r 38e98f5e2ca963fb2dd35af1c62c534dbe62e302

Comment 12

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

https://reviewboard.mozilla.org/r/3301/#review2707

::: browser/devtools/webide/test/head.js
(Diff revision 1)
> -const {Promise: promise} = Cu.import("resource://gre/modules/devtools/deprecated-sync-thenables.js", {});

I did not realize until investigating this that we were using sync promises in our tests but async in the product code... D:

Comment 13

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

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

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

https://reviewboard.mozilla.org/r/3167/#review2747

I'm seeing the toolbox stay when I disconnect the phone by unplugging it.
Do you reproduce or is it a conflict with my current patch queue?

::: browser/devtools/framework/toolbox-hosts.js
(Diff revision 2)
> +    }

Ahah, I also fixed that in my browser toolbox patch ;) I'll rebase on top of yours.

::: browser/devtools/webide/content/webide.js
(Diff revision 2)
> +      }).then(() => this._closeToolboxUI()).catch(console.error);

I think we should try to call \_closeToolboxUI no matter what is the result of toolbox.destroy().
So I would put the \`then\` after the \`catch\`

Comment 15

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

https://reviewboard.mozilla.org/r/3167/#review2761

Nice catch, that's yet another scenario! D:

Fixing this requires I also land bug 1128027 to force protocol.js to cleanup plus a target.js tweak I'll include here.

> I think we should try to call \_closeToolboxUI no matter what is the result of toolbox.destroy().
> So I would put the \`then\` after the \`catch\`

Okay, makes sense.

Comment 16

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

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

/r/3169 - Bug 1117067 - Destroy toolbox before disconnect. r=ochameau
/r/3301 - Bug 1117067 - Harden toolbox cleanup paths, async promises for WebIDE tests. r=ochameau
/r/3419 - Bug 1117067 - Don't wait for tab.detach to complete. r=ochameau

Pull down these commits:

hg pull review -r 455d07c4e035e0beae8ddbf4482b5b1d648ac1d8

Comment 17

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

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

Comment 18

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

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

/r/3169 - Bug 1117067 - Destroy toolbox before disconnect. r=ochameau
/r/3301 - Bug 1117067 - Harden toolbox cleanup paths, async promises for WebIDE tests. r=ochameau
/r/3419 - Bug 1117067 - Don't wait for tab.detach to complete. r=ochameau

Pull down these commits:

hg pull review -r d4a48fff1592771f246014684585f8665cc15aa3

Comment 19

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

Fixed test failure.

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

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

With bug 1128027, everything looks good now, thanks!
Also I don't know if that's thanks to these patches or what, but toolbox opening/closing looks faster.

Comment 21

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

Going to land these without bug 1128027 for now, as it fixes the original issue of hitting the "disconnect" button.

Bug 1128027 may take some more time to resolve.

Comment 22

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/cefcab378fa0
remote:   https://hg.mozilla.org/integration/fx-team/rev/8e6b816cac9b
remote:   https://hg.mozilla.org/integration/fx-team/rev/b0c804005c80

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cefcab378fa0
https://hg.mozilla.org/mozilla-central/rev/8e6b816cac9b
https://hg.mozilla.org/mozilla-central/rev/b0c804005c80

Comment 24

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

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

Approval Request Comment
[Feature/regressing bug #]: Unclear, likely recent changes in the inspector
[User impact if declined]: WebIDE toolbox locks up when "disconnect" is used, have to close and reopen WebIDE to proceed
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: Low, DevTools only
[String/UUID change made/needed]: None

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to J. Ryan Stinnett [:jryans] from comment #24)
> [Feature/regressing bug #]: Unclear, likely recent changes in the inspector

Does this issue impact Beta 36?

> [User impact if declined]: WebIDE toolbox locks up when "disconnect" is
> used, have to close and reopen WebIDE to proceed
> [Describe test coverage new/current, TreeHerder]: on m-c

Did you do any specific testing on m-c to ensure that this is actually fixed? Can you verify the fix before uplift?

Comment 26

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #25)
> (In reply to J. Ryan Stinnett [:jryans] from comment #24)
> > [Feature/regressing bug #]: Unclear, likely recent changes in the inspector
> 
> Does this issue impact Beta 36?

Just tested beta 36 to confirm.  It's not affected.

> > [User impact if declined]: WebIDE toolbox locks up when "disconnect" is
> > used, have to close and reopen WebIDE to proceed
> > [Describe test coverage new/current, TreeHerder]: on m-c
> 
> Did you do any specific testing on m-c to ensure that this is actually
> fixed? Can you verify the fix before uplift?

In Nightly 38 (2015-02-11) with the fix, I just verified it is indeed resolved.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557136 [details]
MozReview Request: bz://1117067/jryans

Although there is a workaround (restart WebIDE), this is not ideal. The fix has already been verified on Nightly. Aurora+

Comment 28

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/857055f5c6a9
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0b12195e1e05
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8a7929c589d1

Comment 30

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

Attachment: MozReview Request: Bug 1117067 - Destroy toolbox before disconnect. r=ochameau

Comment 31

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

Attachment: MozReview Request: Bug 1117067 - Don't wait for tab.detach to complete. r=ochameau

Comment 32

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

Attachment: MozReview Request: Bug 1117067 - Harden toolbox cleanup paths, async promises for WebIDE tests. r=ochameau