Closed Bug 1224073 Opened 9 years ago Closed 8 years ago

Errors generated from console evaluation in worker toolbox come through as [Object object]

Categories

(DevTools :: Console, defect)

Product:
DevTools
Component:
Console
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 affected, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox52 --- fixed

People

(Reporter: bgrins, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

reduced-failure-global-worker.patch
2.63 KB, patch
Details | Diff | Splinter Review
worker-error.patch
3.23 KB, patch
Details | Diff | Splinter Review
ignore-parse-errors.patch
3.20 KB, patch
Details | Diff | Splinter Review
Instead of unsafeDereference, use the Debugger.Object API to call Error.toString directly.
4.70 KB, patch
bgrins
: review+
Details | Diff | Splinter Review 
STR:
devtools.debugger.workers = true
Open http://bgrins.github.io/devtools-demos/worker/webworker.html
Open debugger
Click on worker to open toolbox
Switch to console
Run something that causes an error, like `asdf;`

Expected: See 'ReferenceError: asdflkj is not defined'
Actual: See '[Object object]'
See Also: → 1215120
Summary: Errors generated when from console evaluation in worker toolbox come through as [Object object] → Errors generated from console evaluation in worker toolbox come through as [Object object]
A more simple failure case.  It can be seen by executing a command in the console in both a normal toolbox or worker toolbox.

Normal output:
  Running reducedFailure - isWorker? false
  Finished reducedFailure.  Error message: 'Error: Bug 1224073'

Worker output:
  Running reducedFailure - isWorker? true
  EMITTING: emit(newSource, [object Object]) from undefined() -> undefined
  Finished reducedFailure.  Error message: '[object Object]'

Interesting that we see a newSource event appear during the eval
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Interesting that we see a newSource event appear during the eval

Turns out this was just due to emit logging not happening on the main thread
Very interesting.. it all works if instead of doing:

errorMessage = error.unsafeDereference().toString()

I do:

errorMessage = dbgWindow.executeInGlobalWithBindings("_self.toString()",
                                                    {_self: error}).return;

According to the section on unsafeDereference in https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object - "If the referent is an inner object (say, an HTML5 Window object), return the corresponding outer object (say, the HTML5 WindowProxy object).  This makes unsafeDereference more useful in producing values appropriate for direct use by debuggee code, without using invocation functions."

In theory these two things should be equivalent (at least, they seem to be in the main thread).  Eddy, it seems that unsafeDereference() is failing to return an actual object in the case of an Error when evaluating with 'global' as the dbgWindow but not with 'this'.  Other objects work fine.  Is it possible that this has something to do with the security wrapper for the debuggee's global?
ni? for Comment 3, and see the attachment which has a workaround in JS that gets the test to pass
Flags: needinfo?(ejpbruel)
A note - we discussed making debugging OpaqueCrossCompartmentWrapper since it sounds like it might be this security wrapper preventing the error object from being referenced: https://dxr.mozilla.org/mozilla-central/source/js/src/proxy/OpaqueCrossCompartmentWrapper.cpp
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Created attachment 8687351 [details] [diff] [review]
> worker-error.patch
> 
> Very interesting.. it all works if instead of doing:
> 
> errorMessage = error.unsafeDereference().toString()
> 
> I do:
> 
> errorMessage = dbgWindow.executeInGlobalWithBindings("_self.toString()",
>                                                     {_self: error}).return;
> 
> According to the section on unsafeDereference in
> https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object
> - "If the referent is an inner object (say, an HTML5 Window object), return
> the corresponding outer object (say, the HTML5 WindowProxy object).  This
> makes unsafeDereference more useful in producing values appropriate for
> direct use by debuggee code, without using invocation functions."
> 
> In theory these two things should be equivalent (at least, they seem to be
> in the main thread).  Eddy, it seems that unsafeDereference() is failing to
> return an actual object in the case of an Error when evaluating with
> 'global' as the dbgWindow but not with 'this'.  Other objects work fine.  Is
> it possible that this has something to do with the security wrapper for the
> debuggee's global?

Brian and I discussed this over Vidyo. I believe I answered all his questions there. Feel free to reset the ni? if that's not the case Brian.
Flags: needinfo?(ejpbruel)
I did some digging. Here's what I figured out.

Instances of DebuggerObject live in the debugger's compartment. Each instance of DebuggerObject refers to a corresponding object in the debuggee's compartment, called the referent.

DebuggerObject_unsafeDereference takes the referent, rewraps it in the debugger's compartment, and then returns the resulting wrapper. When wrapping anything in a worker, we use the Wrap callback defined in RuntimeService.cpp:857. This callback makes sure we're never trying to wrap anything from the debugger's compartment in the debuggee's compartment. When wrapping something from the debuggee's compartment in the debugger's compartment, it creates an OpaqueCrossCompartmentWrapper.

The purpose of OpaqueCrossCompartmentWrapper is to prevent almost all access to the debuggee's compartment from the debugger's compartment. This is necessary because we don't have XPConnect in workers. The only thing you can do with an OpaqueCrossCompartmentWrapper is unwrap it. This allows the debugger to add the debuggee's global as a debuggee. Doing anything else, such as accessing the toString property on an Error object, will cause the OpaqueCrossCompartmentWrapper to intercept that access, and pretend that the property does not exist.

I've done some debugging, and typing a bogus expression in the webconsole in a worker does indeed result in a call to DebuggerObject_unsafeDereference, followed by a call to OpaqueCrossCompartmentWrapper::getOwnPropertyDescriptor. This seems to confirm the above analysis.

So, now that we know what's wrong, we just need to figure out what's the proper way to work around this. Lets discuss this further on irc.
Is there any workaround for this issue? Just ran into this issue on Firefox Nightly 50.0a1 (2016-07-12), which makes debugging extremely difficult as all errors return [object Object]. 

I can get so far trying different commands and stepping through the service worker debugger (this works great) and even trying commands on the Chrome console first to make sure the syntax is valid and I'm not missing something, but it would be very helpful to see the actual error message. 

For example, I have no idea why "self.clients.matchAll().then(function(a) { console.log(a); });" displays an [object Object] error since the syntax is correct, and it actually *does* print the console result as "Array [ ]". Strangely enough, the result "Array [ ]" is printed in a *separate* console window (the host page's console window output, and not the service worker's console).
(In reply to jasonpang2011 from comment #8)
> Is there any workaround for this issue? Just ran into this issue on Firefox
> Nightly 50.0a1 (2016-07-12), which makes debugging extremely difficult as
> all errors return [object Object].

Sorry, I don't know of any workaround.  Eddy, did we ever come up with an implementation plan for how this could be fixed?
Flags: needinfo?(ejpbruel)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to jasonpang2011 from comment #8)
> > Is there any workaround for this issue? Just ran into this issue on Firefox
> > Nightly 50.0a1 (2016-07-12), which makes debugging extremely difficult as
> > all errors return [object Object].
> 
> Sorry, I don't know of any workaround.  Eddy, did we ever come up with an
> implementation plan for how this could be fixed?

Apologies for the late reply. I've been on PTO last week.

If memory serves, the plan was to give each worker it's own console. For this to work, we need to make the console API thread safe, so it can be used as part of the worker debugger API. Last time I checked, baku was working on this.

Baku, what is the status on making the console API thread safe?

(Note that Baku is on PTO until the 12th. It will take some time before we'll get a reply).
Flags: needinfo?(ejpbruel) → needinfo?(amarchesini)
Maybe it's completely unrelated but I see:

JavaScript error: resource://devtools/shared/Parser.jsm, line 10: TypeError: Cu is undefined
EMITTING: emit(newSource, [Actor source/server1.conn5.content-process54/4/8]) from undefined() -> undefined

anytime I write something "wrong" into the console.
Flags: needinfo?(amarchesini)
I don't know the workflow of this code bug the reason why we see [Object object] is because:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#1326

       try {
          ast = Parser.reflectionAPI.parse(aString);
        } catch (ex) {
          ast = {"body": []};
        }

Parser.jsm fails loading. So we go in the catch() block.
This is a temp patch to suppress the warning around parser errors / lexical initialization.  We should still be seeing this issue without that code (although we should make it less noisy in the Parser module regardless)
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > (In reply to jasonpang2011 from comment #8)
> > > Is there any workaround for this issue? Just ran into this issue on Firefox
> > > Nightly 50.0a1 (2016-07-12), which makes debugging extremely difficult as
> > > all errors return [object Object].
> > 
> > Sorry, I don't know of any workaround.  Eddy, did we ever come up with an
> > implementation plan for how this could be fixed?
> 
> Apologies for the late reply. I've been on PTO last week.
> 
> If memory serves, the plan was to give each worker it's own console. For
> this to work, we need to make the console API thread safe, so it can be used
> as part of the worker debugger API. Last time I checked, baku was working on
> this.

AFAIK this is already done with getEvents().  And in this case, we are connected to a webconsole actor running inside of the worker thread.
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > (In reply to jasonpang2011 from comment #8)
> > > Is there any workaround for this issue? Just ran into this issue on Firefox
> > > Nightly 50.0a1 (2016-07-12), which makes debugging extremely difficult as
> > > all errors return [object Object].
> > 
> > Sorry, I don't know of any workaround.  Eddy, did we ever come up with an
> > implementation plan for how this could be fixed?
> 
> Apologies for the late reply. I've been on PTO last week.
> 
> If memory serves, the plan was to give each worker it's own console. For
> this to work, we need to make the console API thread safe, so it can be used
> as part of the worker debugger API. Last time I checked, baku was working on
> this.
> 
> Baku, what is the status on making the console API thread safe?
> 
> (Note that Baku is on PTO until the 12th. It will take some time before
> we'll get a reply).

(In reply to Andrea Marchesini [:baku] from comment #11)
> Maybe it's completely unrelated but I see:
> 
> JavaScript error: resource://devtools/shared/Parser.jsm, line 10: TypeError:
> Cu is undefined
> EMITTING: emit(newSource, [Actor
> source/server1.conn5.content-process54/4/8]) from undefined() -> undefined
> 
> anytime I write something "wrong" into the console.

I just talked with Baku about this. Giving each worker its own console won't solve the problem. The root problem is still that we try to access the error object directly, by doing an unsafeDereference, and then calling toString on the resulting object. That will work on the main thread, but in workers, OpaqueCrossCompartmentWrapper will not allow the access to the toString property (or any property for that matter), which is why you're seeing [Object object].

However, looking at that code, I'm wondering why we need to do an unsafeDereference here at all? If the goal is to get to the toString property of the error object, we could just call getOwnPropertyDescriptor on the Debugger.Object wrapper for the error to obtain a Debugger.Object wrapper for its toString property, and then call that. The debugger API itself can bypass the OpaqueSecurityWrapper, so this would avoid the problem described in comment 7.

Brian, is there any reason why this approach would not work? If you can't think of anything, I will start writing a patch for this.
Assignee: nobody → ejpbruel
Flags: needinfo?(bgrinstead)
Good idea - I don't see why this wouldn't work
Flags: needinfo?(bgrinstead)
Attached patch WIP (obsolete) — — Splinter Review 
I made a first attempt at this, but even when I call the toString method on the error object via the debugger API I end up with [Object object]. I am not yet sure why.

Since Error.prototype.toString is implemented as self-hosted code, I suspected that the OpaqueSecurityWrapper prevents the toString method from accessing the name and message fields on the error objects. However, since we enter the debuggee compartment for the call, there should be no wrappers involved at all.

I will have to debug this code to figure out what exactly is going on.
Comment on attachment 8784357 [details] [diff] [review]
WIP

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

Just a heads up: there's a 'todo' in an existing mochitest to cover this case: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_worker-console-02.js#46-48

::: devtools/server/actors/webconsole.js
@@ +901,3 @@
>          errorGrip = this.createValueGrip(error);
> +
> +        errorMessage = String(error);

This reminds me - I had a workaround in Comment 3:

  errorMessage = dbgWindow.executeInGlobalWithBindings("_self.toString()",
                                      {_self: error}).return;

(We'd need to also pull out some of the frameactor code currently in evalWithDebugger into a function that could be called in both places to make sure we are talking to the right Debugger)

I can't remember why we didn't land that code - I think this led us into debugging why unsafeDereference() didn't work, but if this workaround ends up fixing the bug then at this point I'm thinking we should land it and figure out what's going on with the OpaqueSecurityWrapper in a follow up.
Comment on attachment 8784938 [details]
Bug 1224073 - Instead of unsafeDereference to get the string of an error in a worker, evaluate it in Debugger;

Clearing review while sorting out a failure on try
Attachment #8784938 - Flags: review?(ejpbruel)
Brian, I finally figured out why my patch didn't work. I naively assumed that toString.call() would return its result directly, but in the debugger API, the call method returns a completion value instead. I was stringifying the completion value (which is an object) instead of the actual result, which is why we were still seeing [Object object].

That leaves us with two options. We can either land your patch or mine. Personally I think my patch more directly expresses the intent (i.e. don't use eval where a call will do), but I don't want to make that decision unilaterally.

If you are okay with landing my patch instead of yours, you can go ahead and review it. Otherwise, feel free to cancel the r?, and we can discuss further on irc or in the bug.

Thanks!
Attachment #8784357 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8785930 - Flags: review?(bgrinstead)
Comment on attachment 8785930 [details] [diff] [review]
Instead of unsafeDereference, Use the Debugger.Object API to call Error.toString directly.

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

I'm fine with this in general - please also update browser_dbg_worker-console-02.js to get rid of the todo

::: devtools/server/actors/webconsole.js
@@ +901,4 @@
>          errorGrip = this.createValueGrip(error);
> +
> +        errorMessage = String(error);
> +        if (typeof error === "object" && error !== null) {

Can you break this part out into a utility function so this code is easier to read?  We have some similar functions here and we could add a new thing there it's generally useful: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#165.

I think if we had a function that takes in a Debugger.Object and a function name, then calls the function and returns the result it'd be generally useful, so something like:

  errorMessage = String(error);
  let result = DevToolsUtils.getFunctionResult(error, "toString")`;
  if (result !== null && "return" in result) {
    errorMessage = result.return;
  }

What do you think?
Attachment #8785930 - Flags: review?(bgrinstead) → feedback+
Comment on attachment 8784938 [details]
Bug 1224073 - Instead of unsafeDereference to get the string of an error in a worker, evaluate it in Debugger;

Clearing the review request for my patch
Flags: needinfo?(bgrinstead)
Attachment #8784938 - Flags: review?(ejpbruel)
(In reply to Brian Grinstead [:bgrins] from comment #24)
> Comment on attachment 8785930 [details] [diff] [review]
> Instead of unsafeDereference, Use the Debugger.Object API to call
> Error.toString directly.
> 
> Review of attachment 8785930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with this in general - please also update
> browser_dbg_worker-console-02.js to get rid of the todo
> 
> ::: devtools/server/actors/webconsole.js
> @@ +901,4 @@
> >          errorGrip = this.createValueGrip(error);
> > +
> > +        errorMessage = String(error);
> > +        if (typeof error === "object" && error !== null) {
> 
> Can you break this part out into a utility function so this code is easier
> to read?  We have some similar functions here and we could add a new thing
> there it's generally useful:
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/DevToolsUtils.
> js#165.
> 
> I think if we had a function that takes in a Debugger.Object and a function
> name, then calls the function and returns the result it'd be generally
> useful, so something like:
> 
>   errorMessage = String(error);
>   let result = DevToolsUtils.getFunctionResult(error, "toString")`;
>   if (result !== null && "return" in result) {
>     errorMessage = result.return;
>   }
> 
> What do you think?

Sounds like a good idea to me. I'll update the test and factor out the utility function you suggested.
New patch, with comments by bgrins addressed.
Attachment #8784938 - Attachment is obsolete: true
Attachment #8785930 - Attachment is obsolete: true
Attachment #8786405 - Flags: review?(bgrinstead)
Comment on attachment 8786405 [details] [diff] [review]
Instead of unsafeDereference, use the Debugger.Object API to call Error.toString directly.

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

I'd request to have a new test for this util function before landing anything, but I can't find a good place where we are already covering these utility functions.  If you are able add one that'd be great (maybe very similar to test_safe-getter.js but I guess in the shared/ directory?).  But r=me either way so we can get this fix out.

::: devtools/server/actors/webconsole.js
@@ +901,4 @@
>          errorGrip = this.createValueGrip(error);
> +
> +        errorMessage = String(error);
> +        if (typeof error === "object" && error !== null) {

I think `if (error)` would do just as well in this case.

Alternatively, do we even need this `if`?  If it's null then it'll just throw in the call to DevToolsUtils.callPropertyOnObject and be caught there.

::: devtools/shared/DevToolsUtils.js
@@ +637,5 @@
> +    }
> +    proto = proto.proto;
> +  } while (proto !== null);
> +  if (descriptor === undefined) {
> +    throw new Error("No such property");

Nit: `No such property: ${name}`
Attachment #8786405 - Flags: review?(bgrinstead) → review+
Hi Eddy, any chance you can wrap up this patch and get it landed before the merge (it's pretty much ready to go - just see Comment 28)?
Flags: needinfo?(ejpbruel)
I'm on it.
Flags: needinfo?(ejpbruel)
The previous try push for this patch had test failures.

Before this patch, we would always call toString on the error object from the debugger's compartment. This causes issues in workers, where objects in the debuggee can never be accessed from the debugger's compartment. To fix this, this patch changed the behavior to always call toString from the debuggee's compartment.

Unfortunately, it turns out that some of our tests assume that toString will be called from the debugger's compartment, so they expect a valid string representation even if the debuggee cannot access the error object. To accomodate these tests, I've refactored the patch as follows: first try to call toString from the debuggee's compartment, and only if that fails try to call toString from the debugger's compartment.

Here's a new try push with those changes included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06faaf3f30fa
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be9378df21
Use the Debugger.Object API to call Error.toString directly. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/c5be9378df21
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug with Nightly 45.0a1 (2015-11-11) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Nightly  

Build ID   : 20160928030201
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: