827083 - Cannot attach remote web console to Firefox Android

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

[Environment]
* Desktop: http://hg.mozilla.org/mozilla-central/rev/d8ca3e1c469e
* Firefox Android: http://hg.mozilla.org/mozilla-central/rev/d8ca3e1c469e

[Step to reproduce]
Try to attach to remote web console to Firefox Android.

[Result]
This error is shown in desktop error console: 
Error: TypeError: aTab is undefined
Source File: resource://gre/modules/HUDService.jsm
Line: 105

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I can confirm this. It looks like a recent regression.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Stack trace:

HS_activateHUDForContext@resource:///modules/HUDService.jsm:105
StyleEditor_open@resource:///modules/WebConsolePanel.jsm:36
webConsoleDefinition.build@resource:///modules/devtools/ToolDefinitions.jsm:79
TBOX_selectTool/boundLoad<@resource:///modules/devtools/Toolbox.jsm:436

JavaScript error: resource:///modules/HUDService.jsm, line 106: TypeError: aTab is undefined

Comment 3

[Mihai Sucan [:msucan]]

Went through hg blame and I found bug 822609. It seems this.target.tab is undefined in WebConsolePanel open(). This might also affect connections to b2g. Going to investigate more.

Tetsuharu: thank you for the bug report.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #3)
> Went through hg blame and I found bug 822609. It seems this.target.tab is
> undefined in WebConsolePanel open(). This might also affect connections to
> b2g. Going to investigate more.

Note that in bug 799151 the web console and JS debugging are going to be disabled for B2G due to product concerns, so you might as well concentrate on Fennec for now.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch gets rid of a lot of code in HUDService.jsm.

The patch from bug 822609 rightly removed getHostTab() and no longer gave the a Web Console a xul:tab reference for RemoteTargets. The Web Console still relies on a local xul:tab for small UI bits.

Changes prompted by the aove:
- I had to drop the |tab| argument from activateHUDForContext() and from other places in the Web Console.
- since I break the API, I decided I won't monkey-patch HUDService - so I entirely removed activate/deactivateHUDForContext() and a bunch of other code. Kept only a lightweight openWebConsole(target, iframe) in HUDService.
- I switched to Promises for init/destroy and simplified WebConsolePanel.
- I also cleaned-up the initialization:
  - onWindowUnload() in HUDService no longer served any purpose. Removed.
  - ditto for sequencer.
  - wakeup/suspend/shutdown and the WebConsoleObserver are also superfluous. Removed. The Toolbox covers all of these needs - and the HUDService no longer does it correcttly.
  - removed stuff related to console positioning that affected initialization.
- removed toggleRemoteHUD() - it seems unused.

Please let me know if I missed anything. All tests pass here, and it works with b2g desktop and fennec. Thank you!

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=6853eb4d429c

For Aurora we can do a very small patch, if we want.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: patch for aurora

Minimal patch for aurora.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: patch v.2 - test fixes

Yesterday's try push showed two errors. This patch should fixed them.

https://tbpl.mozilla.org/?tree=Try&rev=9b8bd361f4f5

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 699993 [details] [diff] [review]
patch v.2 - test fixes

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

::: browser/devtools/commandline/test/browser_cmd_calllog.js
@@ -45,5 @@
>      outputMatch: /No call logging/,
>    });
>  
>    let hud = null;
> -  var onWebConsoleOpen = DeveloperToolbarTest.checkCalled(function(aSubject) {

checkCalled isn't optional.

Command line tests are getting more and more asynchronous, and we track functions that are outstanding by wrapping them in DeveloperToolbarTest.checkCalled. The danger of not doing this is that the test will complete before they are called.

The simple rule is:
if you are using DeveloperToolbarTest.test then
  all functions that:
    - are executed asynchronously and
    - are not called directly by DeveloperToolbarTest
  should be wrapped in DeveloperToolbarTest.checkCalled
so we know about it and don't finish early.

This shouldn't affect the code below, but I thought it worth mentioning.

@@ +67,5 @@
>  
>      let labels = hud.outputNode.querySelectorAll(".webconsole-msg-output");
>      is(labels.length, 0, "no output in console");
>  
> +    executeSoon(function() {

Can you remember what the error was when this was missing?
If not, I can apply and hack.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #8)
> Comment on attachment 699993 [details] [diff] [review]
> patch v.2 - test fixes
> 
> Review of attachment 699993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/test/browser_cmd_calllog.js
> @@ -45,5 @@
> >      outputMatch: /No call logging/,
> >    });
> >  
> >    let hud = null;
> > -  var onWebConsoleOpen = DeveloperToolbarTest.checkCalled(function(aSubject) {
> 
> checkCalled isn't optional.
> 
> Command line tests are getting more and more asynchronous, and we track
> functions that are outstanding by wrapping them in
> DeveloperToolbarTest.checkCalled. The danger of not doing this is that the
> test will complete before they are called.
> 
> This shouldn't affect the code below, but I thought it worth mentioning.

Thanks for looking into the patch. checkCalled() was not used in the other calllog test, hence I removed it here.

The problem, for me, was that checkCalled() calls finish() too soon. Shall I put it back?

> @@ +67,5 @@
> >  
> >      let labels = hud.outputNode.querySelectorAll(".webconsole-msg-output");
> >      is(labels.length, 0, "no output in console");
> >  
> > +    executeSoon(function() {
> 
> Can you remember what the error was when this was missing?
> If not, I can apply and hack.

It looked like hud.destroy() was not called - because even if the toolbox closed and the tests apparently were all passing, I still had a debugger connection dangling after the test. This is actually how I found the problem: I checked for the debugger's connection count after each test.

Another thing I noticed looking into the code now: notifyObservers() executes in sync (I know of this "feature" from other times). If you look at WCF__initConnection() in webconsole.js, the notification is sent in onSucces, right before the promise is resolved. Perhaps calling notifyObservs() in a setTimeout(..., 0) would "fix" the problem? Or just calling notifyObservers() after I call resolve().

I will investigate this issue as well.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 699948 [details] [diff] [review]
patch for aurora

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

Looking good.

Comment 11

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #9)
> (In reply to Joe Walker [:joe_walker] [:jwalker] from comment #8)
> > Comment on attachment 699993 [details] [diff] [review]
> > patch v.2 - test fixes
> > 
> > Review of attachment 699993 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/commandline/test/browser_cmd_calllog.js
> > @@ -45,5 @@
> > >      outputMatch: /No call logging/,
> > >    });
> > >  
> > >    let hud = null;
> > > -  var onWebConsoleOpen = DeveloperToolbarTest.checkCalled(function(aSubject) {
> > 
> > checkCalled isn't optional.
> > 
> > Command line tests are getting more and more asynchronous, and we track
> > functions that are outstanding by wrapping them in
> > DeveloperToolbarTest.checkCalled. The danger of not doing this is that the
> > test will complete before they are called.
> > 
> > This shouldn't affect the code below, but I thought it worth mentioning.
> 
> Thanks for looking into the patch. checkCalled() was not used in the other
> calllog test, hence I removed it here.

Yes. In that case the tests themselves decide when we're finished (see executeSoon(finish)).

> The problem, for me, was that checkCalled() calls finish() too soon. Shall I
> put it back?

I think we should, probably.
And I'd guess that we need to add a checkCalled around your new executeSoon function since we know that's going to be async. Does that fix the problem?

> > @@ +67,5 @@
> > >  
> > >      let labels = hud.outputNode.querySelectorAll(".webconsole-msg-output");
> > >      is(labels.length, 0, "no output in console");
> > >  
> > > +    executeSoon(function() {
> > 
> > Can you remember what the error was when this was missing?
> > If not, I can apply and hack.
> 
> It looked like hud.destroy() was not called - because even if the toolbox
> closed and the tests apparently were all passing, I still had a debugger
> connection dangling after the test. This is actually how I found the
> problem: I checked for the debugger's connection count after each test.

Hmmm. I'll have a think. My gut reaction is that executeSoon isn't a great hardship in this case, and that the requirement to close a toolbox directly after it has opened, isn't generally useful.

> Another thing I noticed looking into the code now: notifyObservers()
> executes in sync (I know of this "feature" from other times). If you look at
> WCF__initConnection() in webconsole.js, the notification is sent in
> onSucces, right before the promise is resolved. Perhaps calling
> notifyObservs() in a setTimeout(..., 0) would "fix" the problem? Or just
> calling notifyObservers() after I call resolve().
> 
> I will investigate this issue as well.

Thanks,

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 699993 [details] [diff] [review]
patch v.2 - test fixes

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

Looks good, nice cleanups!

::: browser/devtools/framework/Toolbox.jsm
@@ +488,5 @@
>      if (!Hosts[hostType]) {
>        throw new Error('Unknown hostType: '+ hostType);
>      }
> +    // Warning: not all Targets have a tab property - make sure you correctly
> +    // mix and match hosts and targets.

Maybe you should move the warning to the method comment or add a sibling there. Users are more likely to read API than implementation comments.

::: browser/devtools/webconsole/HUDService.jsm
@@ +428,5 @@
>    {
> +    let window = HUDService.currentContext();
> +    let target = TargetFactory.forTab(window.gBrowser.selectedTab);
> +    let toolbox = gDevTools.getToolbox(target);
> +    let panel = toolbox ? toolbox.getPanel("webconsole") : null;

I'm not sure under what circumstances |panel| would be null, but won't we leak a TabTarget in those cases?

::: browser/devtools/webconsole/webconsole.js
@@ +4175,4 @@
>        }.bind(this));
>      }
> +
> +    return this._connectDefer.promise;

Previously you created a |promise| variable in the local scope that you can reuse here.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: patch for fx-team (v3)

Thank you for your review Panos!

Landed:
https://hg.mozilla.org/integration/fx-team/rev/920997d6cb2d

Included the changes you suggested, except I forgot the getOpenHUD() fix. I will submit a separate small fix.

I also did a fix for the promises thing discussed with Joe. This patch no longer changes the GCLI tests - I call notifyObservers() after promise resolve() in WCF__initConnection().

Comment 14

[Mihai Sucan [:msucan]]

Attachment: quick fix for fx-team

This fixes getOpenHUD(). I forgot to address the review comment. Sorry!

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 701197 [details] [diff] [review]
quick fix for fx-team

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

OK!

Comment 16

[Mihai Sucan [:msucan]]

Landed the quick fix:
https://hg.mozilla.org/integration/fx-team/rev/83063b8f405d

Thank you Panos!

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 699948 [details] [diff] [review]
patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 822609.
User impact if declined: the remote Web Console will not work with Firefox for Mobile.
Testing completed (on m-c, etc.): pushed to try [1] and local testing.
Risk to taking this patch (and alternatives if risky): minimal to no risk.
String or UUID changes made by this patch: none.

[1] https://tbpl.mozilla.org/?tree=Try&rev=484f65d89cac

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/920997d6cb2d
https://hg.mozilla.org/mozilla-central/rev/83063b8f405d

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 699948 [details] [diff] [review]
patch for aurora

Firefox 20 regression preventing remote web console usage with Firefox for Android. Approving for Aurora 20.

Comment 20

[Mihai Sucan [:msucan]]

Thank you Alex!

Landed in aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/513c4fe7baf8