Closed Bug 1117067 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: WebIDE, defect)

37 Branch
x86_64
Linux
defect
Not set

Tracking

(firefox36 unaffected, firefox37 fixed, firefox38 fixed)

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- unaffected
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jlorenzo, Assigned: jryans)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Do you have the same issue on your Mac, Martijn?
Flags: needinfo?(martijn.martijn)
Yes, I can reproduce.
Thanks for filing, I had issues, where webIDE randomly seemed to stop working. I think that's the same issue.
Flags: needinfo?(martijn.martijn)
Johan, could you check if there are any errors in the Browser Console?

This will help find the cause of the issue.
Flags: needinfo?(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?
Flags: needinfo?(jlorenzo) → needinfo?(jryans)
(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
Flags: needinfo?(jryans) → needinfo?(jlorenzo)
Attached video 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.
Flags: needinfo?(jlorenzo)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1117067/jryans (obsolete) —
/r/3169 - Bug 1117067 - Destroy toolbox before disconnect. r=ochameau

Pull down this commit:

hg pull review -r 5b49a9202862a1e8a2e7a67217dda27da764daa3
Attachment #8557136 - Flags: review?(poirot.alex)
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
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
Attachment #8557136 - Flags: review?(poirot.alex) → review+
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 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
Attachment #8557136 - Flags: review+ → review?(poirot.alex)
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 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\`
Attachment #8557136 - Flags: review?(poirot.alex)
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.
Depends on: 1128027
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
Attachment #8557136 - Flags: review?(poirot.alex)
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 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.
Attachment #8557136 - Flags: review?(poirot.alex) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/cefcab378fa0
https://hg.mozilla.org/mozilla-central/rev/8e6b816cac9b
https://hg.mozilla.org/mozilla-central/rev/b0c804005c80
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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
Attachment #8557136 - Flags: approval-mozilla-aurora?
(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?
Flags: needinfo?(jryans)
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jryans)
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+
Attachment #8557136 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8557136 - Attachment is obsolete: true
Attachment #8619043 - Flags: review+
Attachment #8619044 - Flags: review+
Attachment #8619045 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.