Closed Bug 1294355 Opened 3 years ago Closed 3 years ago

Error when opening link from stack trace: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox50 fixed, firefox51 verified)

VERIFIED FIXED
Firefox 51
Tracking Status
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: magicp.jp, Assigned: jsnajdr)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160810030202

Steps to reproduce:

1. Start Nightly
2. Open Browser Console (Ctrl+Shift+J)
3. Find any errors (or using steps to reproduce in Bug 1294314)
4. Open frame-link-source-inner (open twisty)
5. Hyperlink frame-link-filename


Actual results:

The following error occurs and a new blank tab will be opened.
Error: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=24530e316b12694eebf9f853d6f7800f7656b579&tochange=867a287c1fe16094f69ff40fa8a1b4d77efa030d


Expected results:

view-source should be opened.
Blocks: 670002
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Console
OS: Unspecified → All
Hardware: Unspecified → All
Keywords: regression
Priority: -- → P1
Summary: Error: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13 → Error when opening link from stack trace in browser console: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13
This isn't just in the browser console - it happens with stack traces in the web console too and can be seen at https://bgrins.github.io/devtools-demos/console/all.html.

For whatever reason it seems like this line needs to be frame.url and not frame.source [0], although from a look at the stack-trace component I'm not sure why since frame seems to define it as source [1].  Jarda, do you happen to know what's going on here?

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3556.
[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/stack-trace.js#53
Flags: needinfo?(jsnajdr)
Summary: Error when opening link from stack trace in browser console: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13 → Error when opening link from stack trace: Must supply a URL when opening view source ViewSourceBrowser.jsm:185:13
(In reply to Brian Grinstead [:bgrins] from comment #1)
> For whatever reason it seems like this line needs to be frame.url and not
> frame.source [0], although from a look at the stack-trace component I'm not
> sure why since frame seems to define it as source [1].  Jarda, do you happen
> to know what's going on here?

These frame objects got a bit messed up when sourcemapping was added. See how onClick is called with an object mapped by getSource at [1], and how the getSource and getFrame methods map between two representations of the frame object.
Assignee: nobody → jsnajdr
Flags: needinfo?(jsnajdr)
The "frame" object passed to the Frame and StackTrace React components has format {source, line}. The object passed to the onClick callback at [1] used to have the same format, but was changed to {url, line} in bug 670002. One usage at [2] was updated, but the other one at [3] wasn't.

This is fixed in file devtools/client/webconsole/console-output.js

There are few other drive-by improvements I made in the same patch:

devtools/client/webconsole/webconsole.js
- fixed eslint errors in createLocationNode that were caused by the double destructuring of {url, line} (variable already declared in upper scope)

devtools/client/webconsole/webconsole.js
devtools/client/shared/components/stack-trace.js
- detect if the source URL is in format "url1 -> url2" (happens all the time in browser toolbox), and pass just "url2" to the components

devtools/client/shared/components/frame.js
- add getInitialState method, lets us avoid checking if this.state is null
- subscribe to the sourceMapService in componentDidMount instead of componentWillMount. This is the recommended place [4] for triggering AJAX-like requests
- if the component props are changed, update the sourcemapping correctly by adding componentWillReceiveProps
- remove the confusing defaulting of argument from getSource - make it a pure function that depends only on its input
- renamed state.frame to state.sourceMappedFrame - I had trouble to understand what's the difference between props.frame and state.frame
- removed state.isSourceMapped - the presence of sourceMappedFrame is sufficient


[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/components/frame.js#225
[2] http://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#2544
[3] http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3556
[4] https://facebook.github.io/react/docs/component-specs.html#mounting-componentdidmount
Comment on attachment 8781978 [details]
Bug 1294355 - Pass correct arguments to openLocationInDebugger in console stack trace

https://reviewboard.mozilla.org/r/72280/#review69948

Thanks for tracking this down and fixing it.  A couple of questions below

::: devtools/client/shared/components/frame.js:70
(Diff revision 1)
>    },
>  
> +  /**
> +   * Update the sourcemapping subscription when the frame location changes
> +   */
> +  componentWillReceiveProps(newProps) {

This change looks like an unrelated fix that can be done in a follow up bug (esp since we will need to uplift this patch to fix view source).

Is there a particular STR where this is happening / causing a problem right now?  And if so, can you add a regression test for it?

::: devtools/client/shared/components/stack-trace.js:53
(Diff revision 1)
>        }
>  
>        frames.push(Frame({
>          frame: {
>            functionDisplayName: s.functionName,
> -          source: s.filename,
> +          source: s.filename.split(" -> ").pop(),

I can't find any tests for the stack-trace component (are there any?), it'd be nice if we could have a regression test for this change
Attachment #8781978 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> > +  componentWillReceiveProps(newProps) {
> 
> This change looks like an unrelated fix that can be done in a follow up bug
> (esp since we will need to uplift this patch to fix view source).

OK, I'm submitting only a minimal fix now and will land the other improvements in separate bugs.

> Is there a particular STR where this is happening / causing a problem right
> now?  And if so, can you add a regression test for it?

I'm not sure if I could reproduce this by actually doing something in the devtools UI today, but it's the kind of bug that's obvious when looking at the code and it's going to bite someone one day. When the Frame component shows a sourcemapped location, and the location is changed, then sometimes it will continue to show the old location. But anyway, I'll create a separate bug for this.
Comment on attachment 8781978 [details]
Bug 1294355 - Pass correct arguments to openLocationInDebugger in console stack trace

https://reviewboard.mozilla.org/r/72280/#review70286

Thanks!  Please update the commit message to say what it's doing i.e. 'Fix error when...'
Attachment #8781978 - Flags: review?(bgrinstead) → review+
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/805d61a1ff2c
Pass correct arguments to openLocationInDebugger in console stack trace r=bgrins
https://hg.mozilla.org/mozilla-central/rev/805d61a1ff2c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8781978 [details]
Bug 1294355 - Pass correct arguments to openLocationInDebugger in console stack trace

Approval Request Comment
[Feature/regressing bug #]: 670002
[User impact if declined]: Opening 'view source' links from stack traces in console doesn't work
[Describe test coverage new/current, TreeHerder]: None for this case
[Risks and why]: This patch is pretty small and relatively low risk (only affects clicks on links inside stacktraces in the web console).  The regression was introduced when source maps landed in the console (on aurora) - it was due to an object key name changing from 'source' to 'url', and this patch updates that.
[String/UUID change made/needed]:
Attachment #8781978 - Flags: approval-mozilla-aurora?
Hello magicp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
Comment on attachment 8781978 [details]
Bug 1294355 - Pass correct arguments to openLocationInDebugger in console stack trace

Recent regression in fx50, Aurora+
Attachment #8781978 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Actual results was fixed in latest Nightly (Build ID: 20160822064441). However, Expected Results didn't pass as some hyperlink occurs "File not found". Should I fill a new bug report?

STR
1. Start Nightly
2. Open Browser Console (Ctrl+Shift+J)
3. menubar > File > Work Offline => An error occurs
4. Open frame-link-source-inner (open twisty)
5. Hyperlink all of frame-link-filename > link 6 and 7 > File not found

X v Attempt to stop observing a preference "userAgentID" that's not being observed1Preferences.jsm:323
  Preferences.ignore	resource://gre/modules/Preferences.jsm:323:5
  this.PushServiceWebSocket._shutdownWS	resource://gre/modules/PushServiceWebSocket.jsm:356:5
  this.PushServiceWebSocket.disconnect	resource://gre/modules/PushServiceWebSocket.jsm:147:5
  this.PushService._changeStateOfflineEvent	resource://gre/modules/PushService.jsm:244:9
  observe/<	resource://gre/modules/PushService.jsm:311:11
  Handler.prototype.process	resource://gre/modules/Promise.jsm%20-%3E%20resource://gre/modules/Promise-backend.js:937:23
  this.PromiseWalker.walkerLoop	resource://gre/modules/Promise.jsm%20-%3E%20resource://gre/modules/Promise-backend.js:816:7
  bound	self-hosted
  bound bound	self-hosted
Flags: needinfo?(magicp.jp) → needinfo?(rkothari)
Depends on: 1297329
(In reply to magicp from comment #15)
> Actual results was fixed in latest Nightly (Build ID: 20160822064441).
> However, Expected Results didn't pass as some hyperlink occurs "File not
> found". Should I fill a new bug report?

This is a different bug, unrelated to this one. Filed as bug 1297329. Happens only for chrome scripts (that use nsISubScriptLoader). Thanks for reporting!
Flags: needinfo?(rkothari)
(In reply to Jarda Snajdr [:jsnajdr] from comment #16)

> This is a different bug, unrelated to this one. Filed as bug 1297329.
> Happens only for chrome scripts (that use nsISubScriptLoader). Thanks for
> reporting!

Jarda, Thanks!
I agree that we need a different bug for that and let's consider this one fixed+verified. Thanks magicp for your awesome work! :)
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.