Closed Bug 1258154 Opened 8 years ago Closed 8 years ago

Clicking function in console when debugger is on opens empty tab instead of object inspector

Categories

(DevTools :: Console, defect, P2)

41 Branch
defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Oriol, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [btpp-fix-later])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160319030558

Steps to reproduce:

1. Open Web Console
2. Enter (function(){})
3. Click the returned object


Actual results:

New empty tab is opened.


Expected results:

Object inspect should have inspected the function object.

Browser Console shows this error:
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURI]

Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=89c1cc2faea8903570688dd38d9f71ec0c5f0b87&tochange=256e3e81fed77b17ea9158786a349698bb59a6ab
Blocks: 1132501
Component: Untriaged → Developer Tools
Keywords: regression
Thanks for filing.  I can confirm the issue in a Nightly build.  It looks like it's trying to load the object up in the Debugger instead of the Variables View since it has a location (https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#2781), which results in the new empty tab.

My guess is we need to treat objects who's location was created in a debugger eval frame as if they don't have a location.  James, does that seem right?  If so, how can we check for that?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jlong)
AFAICT this is only affecting functions that are returned from an evaluation in the console, but it's a pretty ugly failure
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Bug 1132501 made this easier to reproduce, but adding an additional step the real culprit seems bug 1050691

1. Open Debugger
2. Open Web Console
3. Enter (function(){})
4. Click the returned object
Blocks: 1050691
Yeah, Bug 1132501 seems like the right issue
No longer blocks: 1050691
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Yeah, Bug 1132501 seems like the right issue

I think you meant bug 1050691
Blocks: 1050691
No longer blocks: 1132501
Component: Developer Tools → Developer Tools: Console
Summary: Clicking function in console opens empty tab instead of object inspector → Clicking function in console when debugger is on opens empty tab instead of object inspector
Version: 46 Branch → 41 Branch
Flags: needinfo?(fayolle-florent)
(In reply to Oriol from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Yeah, Bug 1132501 seems like the right issue
> 
> I think you meant bug 1050691

Oops, thanks.  Clearing the ni? for Florent for now though - let's see what James says about how to fix it.
Flags: needinfo?(fayolle-florent)
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Thanks for filing.  I can confirm the issue in a Nightly build.  It looks
> like it's trying to load the object up in the Debugger instead of the
> Variables View since it has a location
> (https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> console-output.js#2781), which results in the new empty tab.
> 
> My guess is we need to treat objects who's location was created in a
> debugger eval frame as if they don't have a location.  James, does that seem
> right?  If so, how can we check for that?

Even easier: I think we can just check if `location.source.url` is null. If it's null we won't be able to display it in the debugger. Well, we actually could make that work but it would be a lot more work. The debugger should show eval scripts correctly but that path isn't working here.

I'd say just do the quick fix for now: open the variables view if the URL is null.
Flags: needinfo?(jlong)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74311c0248c2
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8733116 [details]
MozReview Request: Bug 1258154 - Minor test cleanups for function inspection in console;r=jlongster

https://reviewboard.mozilla.org/r/41563/#review38209
Attachment #8733116 - Flags: review?(jlong) → review+
Comment on attachment 8733115 [details]
MozReview Request: Bug 1258154 - Open functions in variables view if they were returned from console evaluation;r=jlongster

https://reviewboard.mozilla.org/r/41561/#review38211

::: devtools/client/webconsole/console-output.js:2782
(Diff revision 1)
>      this._text(")");
>    },
>  
>    _onClick: function () {
>      let location = this.objectActor.location;
> -    if (location) {
> +    if (location && location.source && location.source.url) {

When is `location.source` null? If there's a location it should definitely have a source.
Attachment #8733115 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #12)
> Comment on attachment 8733115 [details]
> MozReview Request: Bug 1258154 - Open functions in variables view if they
> were returned from console evaluation;r=jlongster
> 
> https://reviewboard.mozilla.org/r/41561/#review38211
> 
> ::: devtools/client/webconsole/console-output.js:2782
> (Diff revision 1)
> >      this._text(")");
> >    },
> >  
> >    _onClick: function () {
> >      let location = this.objectActor.location;
> > -    if (location) {
> > +    if (location && location.source && location.source.url) {
> 
> When is `location.source` null? If there's a location it should definitely
> have a source.

I bumped into this case when following STR in the bug, so entering `(function(){})` in the console
`location.source.url` should be null. It should have `location.source`, does it not? I'm curious because we should doing this consistently by now. There is a source there, but it just doesn't have a URL. If not I might look into why there is no source (but it's fine to land this with all the checks).
Weird, devtools/client/webconsole/test/browser_webconsole_bug_1050691_click_function_to_source.js failed with this applied and it looks like in that case we need to check for 'location.url' and not 'location.source.url': https://treeherder.mozilla.org/#/jobs?repo=try&revision=74311c0248c2&selectedJob=18379636
Comment on attachment 8733115 [details]
MozReview Request: Bug 1258154 - Open functions in variables view if they were returned from console evaluation;r=jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41561/diff/1-2/
Attachment #8733116 - Attachment description: MozReview Request: Bug 1258154 - Remove extra global in browser_console_variables_view.js;r=jlongster → MozReview Request: Bug 1258154 - Minor test cleanups for function inspection in console;r=jlongster
Comment on attachment 8733116 [details]
MozReview Request: Bug 1258154 - Minor test cleanups for function inspection in console;r=jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41563/diff/1-2/
https://reviewboard.mozilla.org/r/41561/#review39971

::: devtools/client/webconsole/console-output.js:2782
(Diff revisions 1 - 2)
>      this._text(")");
>    },
>  
>    _onClick: function () {
>      let location = this.objectActor.location;
> -    if (location && location.source && location.source.url) {
> +    if (location && IGNORED_SOURCE_URLS.indexOf(location.url) === -1) {

James, this is the change I made to get it to work: https://reviewboard.mozilla.org/r/41561/diff/1-2/.  It's basically making sure that location.url is not "debugger eval code"
Does the change in Comment 20 seem reasonable?
Flags: needinfo?(jlong)
https://reviewboard.mozilla.org/r/41561/#review40073

I see, so the location on objects does not use the same location structure as sources. Hm, I'll think about if that's something we should fix in the future. For now this seems fine. (Sorry for the slow review)
(In reply to Brian Grinstead [:bgrins] from comment #21)
> Does the change in Comment 20 seem reasonable?

Yeah, sorry about that. I didn't realize ObjectActors use a different location object.

MozReview is *so* confusing... I've r+ed it like 3 times but there's still a pending issue? I have no idea. r+ from me on all fronts :)
Flags: needinfo?(jlong)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85199a7b852f
https://hg.mozilla.org/mozilla-central/rev/712a70746d18
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I have reproduced this bug with Nightly 48.0a1 (2016-03-19) on Windows 7 , 64 Bit!

But I could not see the fix on latest Beta 48.0b8 & latest Nightly and Developer Edition




  Build ID    20160714050942

  User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0


  Build ID    20160714030208

  User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0


  Build ID    20160708004052

  User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
(In reply to Maruf Rahman from comment #28)
> I have reproduced this bug with Nightly 48.0a1 (2016-03-19) on Windows 7 ,
> 64 Bit!
> 
> But I could not see the fix on latest Beta 48.0b8 & latest Nightly and
> Developer Edition
> 
> 
> 
> 
>   Build ID    20160714050942
> 
>   User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0)
> Gecko/20100101 Firefox/48.0
> 
> 
>   Build ID    20160714030208
> 
>   User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0)
> Gecko/20100101 Firefox/50.0
> 
> 
>   Build ID    20160708004052
> 
>   User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0)
> Gecko/20100101 Firefox/49.0

Sorry, my mistake. This bug's fix is verified on latest Beta 48.0b8 & latest Nightly and Developer Edition
See Also: → 1297466
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.