1050691 - Click on a function on the console should go to the debugger

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[vito.detullio]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140807004002

Steps to reproduce:

create an empty html file with a tag script pointing to a js file with a function f() { }
open this file html with firefox, open the web developer tools, and show the console.

type
> console.log(f);

the function signature appears in the console, then click on the function name


Actual results:

a property panel shows from the right, showing the function attributes (arguments, caller, length, name, prototype, __proto__)


Expected results:

the focus is moved on the debugger tab, on the function definition.

(this is the same firebug behaviour)

Comment 1

[Florent Fayolle]

I think this is an important firebug-gap. The Chrome DevTools also support this feature.

We should also take care of the use with pretty-print and source maps.

Florent

Comment 2

[Florent Fayolle]

Attachment: 1050691.patch

Here is my WIP.

I managed to get the location of a function using the thread debugger, and push it to the front-end. (Note: using another Debugger object wouldn't retrieve the Debugger.Script object from the function)

So it also means that the location can't be got if the debugger panel haven't been opened before the function has been logged. I think it can be fixed by sending a request to the back-end with the actor ID of the function so it forces the thread debugger to be enabled and responds with its location.

:bgrins, :jlongster: before going further, what do you think about this way of doing?

TODO:
- support the case when the debugger panel wasn't opened before logging the function
- make sure the back-end support the feature (send "location:null" if no location is found but the back-end supports it).
- Tests

Florent

Comment 3

[James Long (:jlongster)]

Nice work!

(In reply to fayolle-florent from comment #2)
> 
> So it also means that the location can't be got if the debugger panel
> haven't been opened before the function has been logged. I think it can be
> fixed by sending a request to the back-end with the actor ID of the function
> so it forces the thread debugger to be enabled and responds with its
> location.
> 

Yeah, we have this problem all the time. We have bug 1132501 to discuss how the debugger panel is initialized. I wouldn't concern yourself with that functionality; just check to see if the grip has a location and open in the debugger if so. Otherwise, open the variables pane like before.

Soon we will fix the debugger initialization and this will just always work. We need to be pretty careful about how to do that because of b2g, where memory is tight. I think we're going to always initialize the debugger, but I think we should look into only doing that if the server is running on desktop (or maybe just *not* do it specifically on b2g).

Comment 4

[James Long (:jlongster)]

Comment on attachment 8605986 [details] [diff] [review]
1050691.patch

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

Looks great, can you also add a test?

::: browser/devtools/webconsole/console-output.js
@@ +2680,5 @@
> +    if (!this.objectActor.location) {
> +      // TODO This case happens when we have received the packet but not
> +      // opened the debugger yet. In this case, we should force the debugger
> +      // to load and request the source location of the function again.
> +      console.error('Open the debugger and type again your function, sorry :(');

If possible, don't error out and just show the variables view like it did before. We will clean this up later when we fix debugger initialization.

Comment 5

[Florent Fayolle]

Attachment: 1050691.patch

Patch updated with tests and context menu for "Open in Variables View".

Thanks in advance for your review :).

Florent

Comment 6

[Florent Fayolle]

I have just seen it raises lots of exceptions when rendering functions on the console of the Browser Content Toolbox, because of the |globalDebugObject| accessor in toolkit/devtools/server/actors/webconsole.js. |threadActor| looks to be undefined in this case. Do you know how to fallback properly?

Florent

Comment 7

[James Long (:jlongster)]

See if checking `_threadActor` on the parent actor works in those cases. I don't fully understand the setup of each kind of browser toolbox, but I noticed that sometimes the actor that stands in for the "tab" defines a `_threadActor` and other times `threadActor`. We should fix that, but try `threadActor || _threadActor` maybe?

Comment 8

[Florent Fayolle]

Sounds a bit better. At least, I don't have any error in the WebConsole of the Browser Toolbox.
Though I get this error:
"console.warn: notDebuggee: cannot access the environment of this function."

I would like to debug the Browser Toolbox. Any tip on how to do so?

Florent

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to fayolle-florent from comment #8)
> I would like to debug the Browser Toolbox. Any tip on how to do so?

Here's what I do:

# Start a normal instance of Firefox with the debugger server running
./mach run -P dev --start-debugger-server 6080
 
# Run the Browser Toolbox instance
./mach run --profile /tmp/foobar -chrome chrome://browser/content/devtools/framework/toolbox-process-window.xul --purgecaches --jsconsole

Then in that second process (the browser toolbox instance) you can have a console for output

Comment 10

[James Long (:jlongster)]

Alex can you take a look at comment 7 and see if that makes any sense? I've noticed that the `threadActor` property is defined inconsistently on the tab-like actors across our toolboxes...

Comment 11

[James Long (:jlongster)]

Comment on attachment 8610125 [details] [diff] [review]
1050691.patch

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

::: browser/devtools/webconsole/test/head.js
@@ +250,4 @@
>    aPopup.addEventListener("popupshown", onPopupShown);
>  
>    info("wait for the context menu to open");
> +  let eventDetails = {type: "contextmenu", button: 2};

nit: remove this change, our style is to have a space after/before the opening/closing brackets

::: toolkit/devtools/server/actors/webconsole.js
@@ +302,5 @@
>  
>    actorPrefix: "console",
>  
> +  get globalDebugObject() {
> +    return this.parentActor.threadActor.globalDebugObject;

Still need to figure out if this is the right way to access the `threadActor`. Hopefully Alex can give us some insight into this.

Comment 12

[Florent Fayolle]

Thanks for the review, James.

> nit: remove this change, our style is to have a space after/before the opening/closing brackets

Yeah. The previous line was a bit unclean (space before but not after). Is it OK that I put spaces for both then?

> Still need to figure out if this is the right way to access the `threadActor`. Hopefully Alex can give us some insight into this.

I made some debug, and actually it appears that the "Browser Content Toolbox" backend doesn't return the |globalDebugObject| object correctly. So +1 for some help, please :).

> Here's what I do:

Works quite well, thanks! I would also need to debug the "Browser Content Toolbox" actors too. Any tips for this?

Florent

Comment 13

[Alexandre Poirot [:ochameau]]

`threadActor` is only set once we attached to the TabActor, which both regular and browser toolbox should do during its opening. But it may easily race if you are trying to access this property too early...

Having said that I don't see any usage of webconsole actor's globalDebugObject in your patch!
(there is only one additional usage of threadActor.globalDebugObject from script.js, but that's different)
So I can't tell you if you are trying to access that too early or not as it doesn't seem to be ever used.

Comment 14

[Florent Fayolle]

> So I can't tell you if you are trying to access that too early or not as it doesn't seem to be ever used.

Oh! That's true... I'll take a look, thanks!

Florent

Comment 15

[Florent Fayolle]

> Having said that I don't see any usage of webconsole actor's globalDebugObject in your patch!

Actually, it is used.

When an evaluated expression returns a Function object, we call |DebuggerServer.ObjectActorPreviewers.Function| (https://github.com/mozilla/gecko-dev/blob/00b71264120daae47afa7d0c7e1428af633f205a/toolkit/devtools/server/actors/script.js#L3973-L4001 ), and pass it the consoleActor as the threadClient. Thus yes, I call the getter here:

> +    let dbgGlobal = threadActor.globalDebugObject;

Note: It has been refactored since bug 1169064, now it is located here with a more appropriate name (hook) \o/:
https://github.com/mozilla/gecko-dev/blob/5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/object.js#L736-L758

-------

> `threadActor` is only set once we attached to the TabActor, which both regular and browser toolbox should do during its opening. But it may easily race if you are trying to access this property too early...

In the Browser Content Toolbox, I call this getter with the "Script" panel initiated and when evaluated an expression that returns a function from the WebConsole. 

Thanks in advance,

Florent

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8610125 [details] [diff] [review]
1050691.patch

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

::: toolkit/devtools/server/actors/script.js
@@ +3806,5 @@
>          userDisplayName.value) {
>        aGrip.userDisplayName = threadActor.createValueGrip(userDisplayName.value);
>      }
>  
> +    let dbgGlobal = threadActor.globalDebugObject;

So this is the place where threadActor is undefined.
It's just that it isn't defined in this scope if I refer to the version you were patching:
https://github.com/mozilla/gecko-dev/blob/00b71264120daae47afa7d0c7e1428af633f205a/toolkit/devtools/server/actors/script.js#L3973-L4001

I don't know exactly how to get a reference to it from here. But james will know for sure!

::: toolkit/devtools/server/actors/webconsole.js
@@ +303,5 @@
>    actorPrefix: "console",
>  
> +  get globalDebugObject() {
> +    return this.parentActor.threadActor.globalDebugObject;
> +  },

I'm sorry to repeat, but really this particular `globalDebugObject` isn't called.
The undefined `threadActor` isn't here.
If I'm wrong, could you please give me the stack of the undefined *and* the stack of this attribute being called. (you can add dump(">> " + new Error.stack + "\n"); to print it)

Comment 17

[Alexandre Poirot [:ochameau]]

We call the previewer functions from here:
  https://github.com/mozilla/gecko-dev/blob/5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/object.js#L124
We pass `this`, which is the ObjectActor instance.

We create ObjectActor() instance from the ThreadActor here:
https://github.com/mozilla/gecko-dev/blob/5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/script.js#L1667
But unfortunately, we do not pass any reference of the ThreadActor to ObjectActor.

James, any idea how to play with script.js abstraction classes to get the threadactor from here?
I don't have particular expertise in script.js, my thing is more the webbrowser.js...

Comment 18

[James Long (:jlongster)]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> We call the previewer functions from here:
>  
> https://github.com/mozilla/gecko-dev/blob/
> 5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/
> object.js#L124
> We pass `this`, which is the ObjectActor instance.
> 
> We create ObjectActor() instance from the ThreadActor here:
> https://github.com/mozilla/gecko-dev/blob/
> 5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/
> script.js#L1667
> But unfortunately, we do not pass any reference of the ThreadActor to
> ObjectActor.
> 
> James, any idea how to play with script.js abstraction classes to get the
> threadactor from here?
> I don't have particular expertise in script.js, my thing is more the
> webbrowser.js...

The code changed extremely recently in git commit 9f7a43f362961cca68dc6e05ec90e673f612da59 (May 30). It's confusing because this patch needs to be rebased.

The thread actor used to be available in these "previewers" but it now isn't... But you are still hitting a different bug I think since you are seeing it locally.

First, we need to work on rebase this patch on top of fx-team and figure out how to get access to the thread actor. Then we can see if there are still bugs with that.

Comment 19

[James Long (:jlongster)]

I think you are going to have to add the `globalDebugObject` to the object created here: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L1667

And in the previewer, you can check if that's available and if so, use it. You wouldn't reference a `threadActor` specifically, just the `globalDebugObject` property.

Comment 20

[Florent Fayolle]

Oh, nice! I am rebasing and then giving you feedback on it. Thanks!

Florent

Comment 21

[Florent Fayolle]

Attachment: 1050691.patch

Patch updated after rebase.

It doesn't work on Browser Toolbox, but fallbacks correctly to the Variable view. However, it raises nasty exceptions on Browser Content Toolbox.

Florent

Comment 22

[Florent Fayolle]

Attachment: stacktrace.txt

Here is the stack of the nasty exception.

Florent

Comment 23

[Florent Fayolle]

> We create ObjectActor() instance from the ThreadActor here:
> https://github.com/mozilla/gecko-dev/blob/5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/script.js#L1667

Actually, we create the ObjectActor object from here (webconsole.js):
https://github.com/mozilla/gecko-dev/blob/5b60b3df81b04a3c6bf87c86a6ae194d4112b77d/toolkit/devtools/server/actors/webconsole.js#L446-L454

> If I'm wrong, could you please give me the stack of the undefined *and* the stack of this attribute 
> being called. (you can add dump(">> " + new Error.stack + "\n"); to print it)

The stack trace is given in comment 22. AS you can see, the exception occurs when accessing the |WebConsoleActor.prototype.globalDebugObject| property.

Florent

Comment 24

[Alexandre Poirot [:ochameau]]

Ok, I'm able to reproduce it now!
It makes much more sense with the rabased patch!!

Just to be clear, it only fails in the browser content toolbox now?
I don't see this error on the browser toolbox.

It looks like, you would just need to rename _threadActor to threadActor in these two files:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/child-process.js#71
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/addon.js#138

So far, none of the tab actors were relying on the thread actor.
Today there is 3 kinds of parent for the tab actors:

  webbrowser.js > BrowserTabActor (which inherits TabActor) used for regular non-e10s tabs
                  RemoteBrowserTabActor used for e10s tabs, but isn't a real parent of tab actors,
                                        it is just a proxy, used from the parent process to
                                        communicate with the client

  chrome.js > ChromeActor (inherits from TabActor)
              used for browser toolbox, to debug the parent process and all chrome stuff

  child-process.js > ChildProcessActor (doesn't inherit from any class)
                     used for the browser content toolbox to debug the chrome of the child
                     processes

  addon.js > BrowserAddonActor (doesn't inherit from any class)
             used for addon toolbox (from about:addons) to debug the javascript of addons

In your case, all the one inheriting from the TabActor were good as they all define threadActor.
You then have to ensure all the other ones to use the same convention.
I'll try to drop some documentation note about these actors!

Comment 25

[James Long (:jlongster)]

That sounds great Alex, thanks for the help. florent, sorry this dragged out a little bit but fixing the above will help a lot with consistency between the toolboxes! I noticed that difference before and meant to follow up on it.

Comment 26

[Florent Fayolle]

> That sounds great Alex, thanks for the help. florent, sorry this dragged out a little bit but fixing the above will help a lot with consistency between the toolboxes! I noticed that difference before and meant to follow up on it.

I am happy to do this. it would indeed prevent any exception to be raised, though this won't make the feature work in the Toolboxes (IIRC, I tried accessing |(this.parentActor.threadActor || this.parentActor._threadActor).globalDebugObject| and it returned no value. I'll check that with some further debugging.

Florent

Comment 27

[James Long (:jlongster)]

Are you sure you clicked on the debugger to initialize it first?

Still, I'm ok if the feature doesn't work in there for now... as long as it doesn't throw an exception.

Comment 28

[Florent Fayolle]

Attachment: 1050691.patch (WIP)

BTW, thanks a lot for your clear explanation and your help Alex! :)

This patch looks to pass the tests of webconsole and not to raise any exception anymore in Browser Content toolbox. 

As I said, it fallbacks to the Variable View in the /Browser (Content)? Toolbox/. But yes, as said James, that's acceptable.

Also if I am not wrong, I should pass the |getGlobalDebugObject| property to all calls of the |ObjectActor| constructor. If so, I would need to rebase again my work as my working directory doesn't have this file:
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/promises.js

I'll try do this job probably before the week-end.

Florent

Comment 29

[Alexandre Poirot [:ochameau]]

Florent, I opened bug 1171416 to document a bit more the actors.
Feel free to provide your feedback there.

Comment 30

[Florent Fayolle]

Attachment: 1050691.patch

Patch updated rebased with the latest source. James, could you review please?

> Florent, I opened bug 1171416 to document a bit more the actors.

Nice, thanks!

Florent

Comment 31

[Florent Fayolle]

Attachment: 1050691.patch

Patch refreshed to just add "r=jlongster" in the header of the patch.

Florent

Comment 32

[James Long (:jlongster)]

Comment on attachment 8616458 [details] [diff] [review]
1050691.patch

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

A bit hard to see exactly what has changed, but I just looked at the placed where you hooked up the global debug object.

Comment 33

[Florent Fayolle]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40373e129ea5

Florent

Comment 34

[Florent Fayolle]

Attachment: 1050691.patch (workaround)

The results in the try-push indicates error because of exceptions raised here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#464

The exception is the following:
> 02:03:44     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "ObjectActor.prototype.grip previewer function threw an exception: TypeError: argument is not a global object
> 02:03:44     INFO -  Stack: ThreadActor.prototype.globalDebugObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:464:12
> 02:03:44     INFO -  ThreadActor.prototype.objectGrip/actor<.getGlobalDebugObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1679:35
> 02:03:44     INFO -  DebuggerServer.ObjectActorPreviewers.Function<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:811:21
> 02:03:44     INFO -  ObjectActor.prototype.grip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:121:15
> 02:03:44     INFO -  ThreadActor.prototype.objectGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1662:14
> 02:03:44     INFO -  ThreadActor.prototype.pauseObjectGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1697:12

I suspect (though not sure) that this comes from the code below from head_dbg.js:
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/head_dbg.js#228-235,239-240

Where the assigned debuggee global is not a global. 

So I made a patch (attached here) with this try-push where everything works perfectly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=710b068837b5

But I think it is not clean. Nick, do you know how to workaround that properly?

Thanks in advance

Florent

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8622088 [details] [diff] [review]
1050691.patch (workaround)

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

::: toolkit/devtools/server/actors/object.js
@@ +42,5 @@
>   *              Increment the actor's grip depth
>   *          - decrementGripDepth
>   *              Decrement the actor's grip depth
> + *          - globalDebugObject
> + *              The Debuggee Global Object as given by the ThreadActor

There can be more than one debuggee global, so talking about *the* debuggee global object doesn't make a ton of sense. Is it ok for an actor implementing this interface to return any debuggee global? Does it need to be a specific one? If so, how do you describe that specific one, and does it make sense across tab-, addon-, and chrome-debugging? (Because it probably needs to.)

::: toolkit/devtools/server/actors/script.js
@@ +466,5 @@
> +    // So detect when the object is not a global and return null.
> +    // xxxFlorent: Something prettier to do?
> +    if (!this._parent.window || !this._parent.window.window) {
> +      return null;
> +    }

https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/testactors.js#98

We probably just need to be smarter about this. I'm not sure why we are faking a funky wrapper, but passing this._global to makeGlobalObjectReference should work fine.

I'd try making that getter just return `this._global` and then see where we are trying to do this weird unwrapping and fix that to handle the case where there isn't wrapping. In fact, I bet that everything will just work, since we have this here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/testactors.js#79

Comment 36

[Florent Fayolle]

Attachment: 1050691.patch

> There can be more than one debuggee global, so talking about *the* debuggee global object doesn't make a 
> ton of sense. Is it ok for an actor implementing this interface to return any debuggee global? Does it 
> need to be a specific one? 

These are excellent questions. Here is my specific need: get the location (url + startLine) of a function. But not any Debugger.Script objects references their location. And AFAICT, the only trick that allows us doing so is the following, where we need to access the globalDebugObject of the ThreadActor (so yes, it is a specific debuggee global):

::: toolkit/devtools/server/actors/object.js
@@ +813,1 @@
> let script = dbgGlobal.makeDebuggeeValue(obj.unsafeDereference()).script;

Also I tested a bit with IFrames. It fallbacks to the variable view if the function being logged is not from the iframe selected by the frame selector. So I guess it also needs to be the debuggee global where the function is defined.

> If so, how do you describe that specific one, and does it make sense across 
> tab-, addon-, and chrome-debugging? (Because it probably needs to.)

I wondered above how to get the right global in these cases so the feature also works in these contexts, but failed on finding a way for that. If you have a simple tip, I would be very glad to adapt my patch.

> We probably just need to be smarter about this. I'm not sure why we are faking a funky wrapper, but passing this._global to makeGlobalObjectReference should work fine.

Sounds to work. I run the try-push below with the attached patch to confirm that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a59b0c1e871b

Thanks Nick,

Florent

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Some thoughts:

* I don't think it should be too hard to expose the definition site on Debugger.Script directly (which seems like it would simplify a lot of stuff here). If you feel up to diving into some SpiderMonkey code, I can help guide you (maybe as a follow up?).

* Every Debugger.Object has a "global" getter on it, which gives you its global (also wrapped in a Debugger.Object). This should also simplify a lot of this patch, I think.

Comment 38

[Florent Fayolle]

Attachment: 1050691.patch

> * Every Debugger.Object has a "global" getter on it, which gives you its global (also wrapped in a 
> Debugger.Object). This should also simplify a lot of this patch, I think.

As explained yesterday on IRC, we indeed currently need to access a global instanciated by the threadActor. I created bug 1174896 so we can simplify this code later (get rid of the globalDebugObject getter for example).

I just updated the patch to make this comment clearer:
>::: toolkit/devtools/server/actors/object.js
>@@ +42,5 @@
>>   *              Increment the actor's grip depth
>>   *          - decrementGripDepth
>>   *              Decrement the actor's grip depth
>> + *          - globalDebugObject
>> + *              The Debuggee Global Object as given by the ThreadActor

The try-push of comment 36 looks to be successful, so I ask for a check-in.

Florent

Comment 39

[Florent Fayolle]

James, as I don't have the permission to set the check-in flag for this bug, could you do that for me please?

Thanks in advance

Florent

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c451a7f1de44

Comment 41

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/f8d73c4249c4 - unfortunately OS X didn't like that in browser_webconsole_closure_inspection.js, https://treeherder.mozilla.org/logviewer.html#?job_id=3484370&repo=fx-team

Comment 42

[Florent Fayolle]

My bad. As it was referenced in an already existing issue in the try-push, and as it passed in other OS, I thought that it was unrelated to my work. Looking deeper at the logs of the results, it is.

Unfortunately, I don't have a Mac OS machine to work on this. I can iterate by submitting modifications to the try-push server, even if that's not ideal.

So either the |_onContextMenu| (browser/devtools/webconsole/console-output.js) function doesn't attach the "command" event handler correctly, either the context menu item is not clicked by the code below (browser/devtools/webconsole/test/browser_webconsole_closure_inspection.js):
>+    EventUtils.synthesizeMouseAtCenter(openInVarView, {},
>+                                       gWebConsole.iframeWindow);
>+  });

Florent

Comment 43

[Brian Grinstead [:bgrins]]

(In reply to fayolle-florent from comment #42)
> So either the |_onContextMenu|
> (browser/devtools/webconsole/console-output.js) function doesn't attach the
> "command" event handler correctly, either the context menu item is not
> clicked by the code below
> (browser/devtools/webconsole/test/browser_webconsole_closure_inspection.js):
> >+    EventUtils.synthesizeMouseAtCenter(openInVarView, {},
> >+                                       gWebConsole.iframeWindow);
> >+  });

You may need to do:

EventUtils.synthesizeMouseAtCenter(openInVarView,
                                   { type: "contextmenu", button: 2 },
                                   gWebConsole.iframeWindow);

Comment 44

[Florent Fayolle]

Doesn't this piece of code trigger the context menu? We already have displayed the context menu open, and we need to click on the right item, according to these logs:

> 19:22:46     INFO -  5270 INFO onPopupShown
> 19:22:46     INFO -  5271 INFO TEST-PASS | browser/devtools/webconsole/test/browser_webconsole_closure_inspection.js 
>                       | the "Open In Variables View" context menu item should be clickable
> 19:22:46     INFO -  5272 INFO onPopupHidden

Or maybe we select the context menu item but nothing happens. I don't know.

Florent

Comment 45

[Brian Grinstead [:bgrins]]

(In reply to fayolle-florent from comment #44)
> Doesn't this piece of code trigger the context menu? We already have
> displayed the context menu open, and we need to click on the right item,
> according to these logs:

Oh yeah, I didn't look very closely and just assumed you were wanting to open a context menu.  Carry on

Comment 46

[Brian Grinstead [:bgrins]]

FWIW the test does fail locally for me when checking in osx

Comment 47

[Brian Grinstead [:bgrins]]

Try switching it from synthesizeMouseAtCenter to: openInVarView.click()

Comment 48

[Florent Fayolle]

Attachment: 1050691.patch

Thanks Brian! Here is the try-push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea4511f6e18

(I am surprised that the click simulation doesn't work correctly specifically on OS X)

Florent

Comment 49

[Florent Fayolle]

Hope it is fine this time.

Florent

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/df3415259fd5

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_dbg_WorkerActor.attachThread.js timeouts across the board.
https://treeherder.mozilla.org/logviewer.html#?job_id=3510433&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/89736b1c392b

Comment 52

[Eddy Bruel [:ejpbruel]]

Hi Florent. This backout isn't your fault. You just got unlucky that a patch for worker debugging landed right before yours did. On worker threads, Cu is not available, so a debugger server on a worker thread cannot use any code paths that need it. Apparently, your patch changed something on the server that caused the debugger server in test_WorkerActor.attachThread to execute a code path with Cu that wasn't used before.

The solution here is probably to add an explicit check to the offending code path, using Cu when it is available, and some other method when it is not (where other method could be just not supporting certain functionality for now).

Comment 53

[Florent Fayolle]

Hi Eddy,

Thanks, your explanation is very kind of you. I'll probably take a look this week-end then.

Florent

Comment 54

[Eddy Bruel [:ejpbruel]]

(In reply to fayolle-florent from comment #53)
> Hi Eddy,
> 
> Thanks, your explanation is very kind of you. I'll probably take a look this
> week-end then.
> 
> Florent

If you need help with working around the worker debugging issues, feel free to ping me on irc, or needinfo me in the bug!

Comment 55

[Florent Fayolle]

Attachment: 1050691.patch

patch updated (the globalDebugObject getter should return null if the environnement doesn't have any window).

Eddy, although it looks like the bug is fixed, the exception was about this line (executed after a first exception had been raised):
https://github.com/mozilla/gecko-dev/blob/bbb0424b3fe962da6e202ac5200feaaea0001a2e/toolkit/devtools/server/actors/object.js#L122

And DevToolsUtils.reportException accesses the Cu object, hence the exception shown in the logs.

In case of exception, is there any clean workaround to the use of Cu.reportError? (using dump() maybe?)

Florent

Comment 56

[Florent Fayolle]

Attachment: 1050691.patch

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=327ee5e3fab1

@Eddy: Hi! I took the liberty to test whether Cu is defined or not here:
> diff --git a/toolkit/devtools/DevToolsUtils.js b/toolkit/devtools/DevToolsUtils.js
> --- a/toolkit/devtools/DevToolsUtils.js
> +++ b/toolkit/devtools/DevToolsUtils.js
> @@ -48,6 +48,6 @@ exports.safeErrorString = function safeE
>  exports.reportException = function reportException(aWho, aException) {
>    let msg = aWho + " threw an exception: " + exports.safeErrorString(aException);
>  
>    dump(msg + "\n");
>  
> -  if (Cu.reportError) {
> +  if (Cu && Cu.reportError) {

Though I am not sure the dump is enough for your needs (try push logs, etc.).

Florent

Comment 57

[Florent Fayolle]

Hope this time, that would be good. :)

Florent

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b12b24c7f989

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b12b24c7f989

Comment 60

[vito.detullio]

I think I found a case when the log of the functions has the "old" behaviour, specifically, in the web workers.

I have this simple code:

-- index.js --
var worker = new Worker('./worker.js');
worker.onmessage = function(e) {
    console.log(worker.onmessage);
};
worker.postMessage(0);

-- worker.js --
self.onmessage = function(e) {
    console.log(self.onmessage);
    self.postMessage(0);
};

the output is:

function (e) {
    console.log(self.onmessage);
    self.postMessage(0);
}
function worker.onmessage()

Comment 61

[James Long (:jlongster)]

That is because web workers do not have a debugger attached to them. We do not fully support web worker debugging yet (what we have is behind a flag).