Closed Bug 827083 Opened 7 years ago Closed 7 years ago

Cannot attach remote web console to Firefox Android

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox20 fixed, firefox21 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: tetsuharu, Assigned: msucan)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

[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
I can confirm this. It looks like a recent regression.
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
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.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Priority: -- → P1
(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.
Priority: P1 → --
Priority: -- → P1
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #699419 - Flags: review?(past)
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch patch for auroraSplinter Review
Minimal patch for aurora.
Attachment #699948 - Flags: review?(past)
Attached patch patch v.2 - test fixes (obsolete) — Splinter Review
Yesterday's try push showed two errors. This patch should fixed them.

https://tbpl.mozilla.org/?tree=Try&rev=9b8bd361f4f5
Attachment #699419 - Attachment is obsolete: true
Attachment #699419 - Flags: review?(past)
Attachment #699993 - Flags: review?(past)
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.
(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 on attachment 699948 [details] [diff] [review]
patch for aurora

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

Looking good.
Attachment #699948 - Flags: review?(past) → review+
(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 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.
Attachment #699993 - Flags: review?(past) → review+
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().
Attachment #699993 - Attachment is obsolete: true
This fixes getOpenHUD(). I forgot to address the review comment. Sorry!
Attachment #701197 - Flags: review?(past)
Comment on attachment 701197 [details] [diff] [review]
quick fix for fx-team

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

OK!
Attachment #701197 - Flags: review?(past) → review+
Landed the quick fix:
https://hg.mozilla.org/integration/fx-team/rev/83063b8f405d

Thank you Panos!
Whiteboard: [fixed-in-fx-team]
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
Attachment #699948 - Flags: approval-mozilla-aurora?
Blocks: 824016
Blocks: 829913
https://hg.mozilla.org/mozilla-central/rev/920997d6cb2d
https://hg.mozilla.org/mozilla-central/rev/83063b8f405d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
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.
Attachment #699948 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.