Closed Bug 1297329 Opened 3 years ago Closed 3 years ago

In console stack traces, URLs in format "url1 -> url2" cannot be opened in debugger

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(2 files)

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

It happens when the URL is in the "url1 -> url2" format (nsISubScriptLoader does that when loading resource:// and file:// URLs). We need to extract the "url2" part and open that.
Assignee: nobody → jsnajdr
Blocks: 1294355
Added the split(" -> ").pop() combo at the right places.

Also, cleaned up a eslint error in WebConsoleFrame.createLocationNode (destructuring overrides variables in upper scope, Hungarian notation).

It seems to be quite complex to mochitest this: it can happen only in browser toolbox, requires loading a file:// or resource:// URL with nsISubScriptLoader...
(In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> Added the split(" -> ").pop() combo at the right places.
> 
> Also, cleaned up a eslint error in WebConsoleFrame.createLocationNode
> (destructuring overrides variables in upper scope, Hungarian notation).
> 
> It seems to be quite complex to mochitest this: it can happen only in
> browser toolbox, requires loading a file:// or resource:// URL with
> nsISubScriptLoader...

Yes, let's not worry about adding a mochitest-browser test for this.  But, it would be possible I think to add a new, simpler mochitest-chrome test for the stack trace component (similar to client/shared/components/test/mochitest/test_frame_01.html).  In this test we could render StackTrace directly with props that pass the '->' into the filename and confirm it didn't get rendered out.  The other benefit is we'd now have at least a simple test in place that could be extended for future stack trace development.  What do you think?
Comment on attachment 8783892 [details]
Bug 1297329 - Fix opening 'url1 -> url2' source locations in console

https://reviewboard.mozilla.org/r/73544/#review71462

See Comment 3 - the code is fine, but it is important we add a small mochitest-chrome to cover the rendering of this component when '->' is passed in as a prop (no need to fully simulate this in a mochitest-browser).  Let me know if you don't have time to work on it now - I'd prefer if they land together in this bug
Attachment #8783892 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> > Added the split(" -> ").pop() combo at the right places.
> > 
> > Also, cleaned up a eslint error in WebConsoleFrame.createLocationNode
> > (destructuring overrides variables in upper scope, Hungarian notation).
> > 
> > It seems to be quite complex to mochitest this: it can happen only in
> > browser toolbox, requires loading a file:// or resource:// URL with
> > nsISubScriptLoader...
> 
> Yes, let's not worry about adding a mochitest-browser test for this.  But,
> it would be possible I think to add a new, simpler mochitest-chrome test for
> the stack trace component (similar to
> client/shared/components/test/mochitest/test_frame_01.html).  In this test
> we could render StackTrace directly with props that pass the '->' into the
> filename and confirm it didn't get rendered out.  The other benefit is we'd
> now have at least a simple test in place that could be extended for future
> stack trace development.  What do you think?

Yes, that's a good idea - adding a StackTrace unit test instead of testing end-to-end.

There's another way how to break the source links - logging the messsage from inside eval. Then it adds funny things to the URL. I discovered this when looking at a stack trace from ContentTask.

Thanks for r+, I'll add a Part2 patch when I'm done.
Added a mochitest-chrome for the StackTrace component. Tests the nsISubScriptLoader formatting and async causes.

I changed the checkFrameString function in head.js to accept element instead of a component instance, to make checking of sub-components easier. Also added assertion names where they were missing (you get "undefined assertion name" in test output otherwise).
Comment on attachment 8784339 [details]
Bug 1297329 - Added unit test for the StackTrace component

https://reviewboard.mozilla.org/r/73828/#review71758

Test looks great, thanks!
Attachment #8784339 - Flags: review?(bgrinstead) → review+
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ac259a41f0f7
Fix opening 'url1 -> url2' source locations in console r=bgrins
https://hg.mozilla.org/integration/autoland/rev/1665164036a7
Added unit test for the StackTrace component r=bgrins
https://hg.mozilla.org/mozilla-central/rev/ac259a41f0f7
https://hg.mozilla.org/mozilla-central/rev/1665164036a7
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
See Also: → 1298225
Comment on attachment 8783892 [details]
Bug 1297329 - Fix opening 'url1 -> url2' source locations in console

Approval Request Comment
[Feature/regressing bug #]: 1281732
[User impact if declined]: Source links in browser toolbox console will fail to open in debugger. Requesting uplift mainly because the patch in bug 1298225 requires this one to apply cleanly.
[Describe test coverage new/current, TreeHerder]: New mochitest is added in this bug
[Risks and why]: low: simple change affecting only browser toolbox, well covered by tests.
[String/UUID change made/needed]: none
Attachment #8783892 - Flags: approval-mozilla-beta?
Comment on attachment 8783892 [details]
Bug 1297329 - Fix opening 'url1 -> url2' source locations in console

this patch is needed to fix another bug, Beta50+
Attachment #8783892 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.