865792 - Firefox for Android remote console doesn't evaluate JS expressions

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Screenshot of the web console

STR:
1. Set the devtools.debugger.remote-enabled pref to true on Fx desktop and Fx android, restart them
2. Run adb forward tcp:6000 tcp:6000
3. In Fx desktop, go to tools->web developer->Connect... and hit ok on the connect dialog
4. Hit ok on the device to accept the incoming connection
5. Select a tab from the tab chooser on desktop to get the remote debugger window for that tab
6. Try entering some javascript expression in the web console tab, e.g. "document.getElementsByTagName('h1')".

Expected results:
Some sort of output, in this case the list of h1 elements

Actual results:
No output

I also tried with simple expressions like "true" which on the desktop web console echo "true" back to the console, but in the remote debugger produces no output.

I'm using an Aurora desktop build and a Nightly Fennec build, if that makes any difference.

Comment 1

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

The Browser Console contains this error (reformatted for readability):

[10:22:56.411] Server did not specify an actor, dropping packet: {
	"error": "unknownError",
	"message": "error occurred while processing 'evaluateJS': 
		ReferenceError: TargetFactory is not defined
		JSTermHelpers/<.get@resource://gre/modules/devtools/WebConsoleUtils.jsm:1594
		WCA_evalWithDebugger@chrome://global/content/devtools/dbg-webconsole-actors.js:759
		WCA_onEvaluateJS@chrome://global/content/devtools/dbg-webconsole-actors.js:505
		DSC_onPacket@chrome://global/content/devtools/dbg-server.js:655
		DT__processIncoming/<.run@chrome://global/content/devtools/dbg-transport.js:177
"} @ resource://gre/modules/devtools/dbg-client.jsm:592

I bet this is a regression from a recent devtools patch.

Comment 2

[Mihai Sucan [:msucan]]

This is caused by bug 842672.

It seems that evalInGlobalWithBindings() executes all getters, before they are actually used.

Is there a way to avoid getters from being executed when we provide them as bindings? We would like getters to execute only when the eval string uses those properties.

Comment 3

[Jim Blandy :jimb]

(In reply to Mihai Sucan [:msucan] from comment #2)
> This is caused by bug 842672.
> 
> It seems that evalInGlobalWithBindings() executes all getters, before they
> are actually used.
> 
> Is there a way to avoid getters from being executed when we provide them as
> bindings? We would like getters to execute only when the eval string uses
> those properties.

Oh, hmm. evalInGlobalWithBindings isn't specified to turn accessor properties in the bindings object into 'accessor variables' in the code's environment. It talks about introducing 'variables' with 'values'.

But that's not what you guys need in this context... Hmm. What are these bindings, exactly?

If they're functions, would it work to have the bindings' values be functions like this?

  function forwardToX() { return X.apply(this, arguments); }

where X is the real function to call? Then, X wouldn't be looked up until it was actually called.

Comment 4

[Mihai Sucan [:msucan]]

Thanks for your reply.


(In reply to Jim Blandy :jimb from comment #3)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > This is caused by bug 842672.
> > 
> > It seems that evalInGlobalWithBindings() executes all getters, before they
> > are actually used.
> > 
> > Is there a way to avoid getters from being executed when we provide them as
> > bindings? We would like getters to execute only when the eval string uses
> > those properties.
> 
> Oh, hmm. evalInGlobalWithBindings isn't specified to turn accessor
> properties in the bindings object into 'accessor variables' in the code's
> environment. It talks about introducing 'variables' with 'values'.
> 
> But that's not what you guys need in this context... Hmm. What are these
> bindings, exactly?

You can see the code in dbg-webconsole-actors.js: search for WCA_evalWithDebugger(), WCA__createGlobal() and WCA__getJSTermHelpers(). In WebConsoleUtils.jsm please search for the JSTermHelpers() function.

The bindings object looks as follows:

{
  $: function() { ... return x; },
  print: function() { ... return x; },
  get $0() { return owner.makeDebuggeeValue(x); }
  ...
}

For all non-getter properties I call makeDebuggeeValue(). $0 calls the function only when the getter is executed.

Now it seems that the getter is always executed.


> If they're functions, would it work to have the bindings' values be
> functions like this?
> 
>   function forwardToX() { return X.apply(this, arguments); }
> 
> where X is the real function to call? Then, X wouldn't be looked up until it
> was actually called.

Unfortunately, this won't work, because $0 needs to be a getter property, not a function.

Can we allow getters in binding objects? With a strict rule of getters must always return a debuggee value or else the code throws exceptions when the getter is executed.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Panos: this is a patch that changes the $0 getter to not throw if the jsm import fails. I had to silence the failure, otherwise every JS eval shows error in the browser console.

I tested this patch with a b2g-desktop build. Fennec build failed during linking...

Please let me know if this is fine. The better approach would be to have support in evalInGlobalWithBindings() to not execute getters until they are used by the eval code.

Thanks!

Comment 6

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

Comment on attachment 744121 [details] [diff] [review]
proposed fix

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

(In reply to Mihai Sucan [:msucan] from comment #5)
> Panos: this is a patch that changes the $0 getter to not throw if the jsm
> import fails. I had to silence the failure, otherwise every JS eval shows
> error in the browser console.
> 
> I tested this patch with a b2g-desktop build. Fennec build failed during
> linking...

The advice I got on #mobile was to use the r8e ndk and clobber. This made fennec build for me.

> Please let me know if this is fine. The better approach would be to have
> support in evalInGlobalWithBindings() to not execute getters until they are
> used by the eval code.

I think this is fine for now. We already know $0 is implemented with a hack right now and we plan to fix this after the remote inspector lands.

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +1596,5 @@
> +        target = devtools.TargetFactory.forTab(window.gBrowser.selectedTab);
> +      }
> +      catch (ex) {
> +        // If we report this exception the user will get it in the Browser
> +        // Console every time when she evaluates any string.

Can you add a reference to the bug about fixing $0 to use the future remotable inspector?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #6)
> Comment on attachment 744121 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 744121 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Mihai Sucan [:msucan] from comment #5)
> > Panos: this is a patch that changes the $0 getter to not throw if the jsm
> > import fails. I had to silence the failure, otherwise every JS eval shows
> > error in the browser console.
> > 
> > I tested this patch with a b2g-desktop build. Fennec build failed during
> > linking...
> 
> The advice I got on #mobile was to use the r8e ndk and clobber. This made
> fennec build for me.

Thanks1


> > Please let me know if this is fine. The better approach would be to have
> > support in evalInGlobalWithBindings() to not execute getters until they are
> > used by the eval code.
> 
> I think this is fine for now. We already know $0 is implemented with a hack
> right now and we plan to fix this after the remote inspector lands.

Yes.

> ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
> @@ +1596,5 @@
> > +        target = devtools.TargetFactory.forTab(window.gBrowser.selectedTab);
> > +      }
> > +      catch (ex) {
> > +        // If we report this exception the user will get it in the Browser
> > +        // Console every time when she evaluates any string.
> 
> Can you add a reference to the bug about fixing $0 to use the future
> remotable inspector?

Bug 787975 is already mentioned in the getter description.

Thanks for the r+.

Should we make a patch for Aurora as well? The reporter mentions aurora, but bug 783499 did not land yet in aurora.



Patch landed:
https://hg.mozilla.org/integration/fx-team/rev/13475fab0883

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/13475fab0883