Closed Bug 1566856 Opened 4 months ago Closed 3 months ago

JSONView stopped rendering relative URLs

Categories

(DevTools :: JSON Viewer, defect, P3)

68 Branch
Desktop
All
defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox68 wontfix, firefox69 fixed, firefox70 verified)

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- verified

People

(Reporter: gatopeich, Assigned: Honza)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

I was using "http:/" prefix (single slash) to create links to navigate JSON documents within the same host.
This was working great for being able to navigate requests, for example to get the link to the next page in a partial result JSON, without resorting to absolute links.
Absolute links are troublesome in some setups, e.g. behind a reverse proxy.

Actual results:

Today after a restart, URLs starting with "http:/" are not recognized as links anymore.

Expected results:

Our "http:/<path>" strings should be recognized as links to the <path> within current host:port. It worked for me until this morning.

If such behaviour is not acceptable for some good reason, how can we produce relative links that JSONView will recognize?

The priority flag is not set for this bug.
:Honza, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(odvarko)

It's true that "http:/reports?start=1&limit=1" was recognized as a link. But I can't reproduce the part that it behaved as a relative link within the same host. For me it just loaded http://www.reports.com/?start=1&limit=1

Today after a restart, URLs starting with "http:/" are not recognized as links anymore.

I can reproduce this on my Win10 machine with Firefox Nightly

Honza

Flags: needinfo?(odvarko)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3

STR:

  1. Load the attached JSON file (reports_with_next_link.json) using your local web server
  2. See the "http:/reports?start=1&limit=1" link, it isn't clickable -> BUG

Changing the link within the JSON file to: http://reports?start=1&limit=1 fixes the problem.
The user can click the link and open it in a new tab.

Honza

Thank you for the extra STR Honza.

Regression range results:

  • First bad: build_date: 2019-04-09 changeset: 9982d52f7ba861051b9ac7c3b2ed1465985a38be
  • Last good: build_date: 2019-04-09 23:27:59.061000 changeset: a31032a16330ca069db82160d2203fa7a9737aca
  • Pushlog URL

Affected builds:
680.0.1, 69.0b10, 70.0a1 (2019-08-04).

OS: Unspecified → All
Hardware: Unspecified → Desktop
Has Regression Range: --- → yes
Has STR: --- → yes

@Cristian thanks!

It looks like bug 1318060 could cause the regression

Honza

No, it was https://github.com/firefox-devtools/debugger/commit/5f696ab68cd54585b2b106caad7c1431ae4b2cdc (it landed as part of https://hg.mozilla.org/mozilla-central/rev/ca42d5764b089097bd34b7f6fef770bc46c2d675)

But again, in http:/reports?start=1&limit=1, reports is the host, not a path relative to the current host.

Just talked to Honza, this is a rather low priority bug (hence P3). We don't need to hurry in getting a fix for beta/69, but if we do find one in time for an uplift, we could take it.

Nicolas, the following patch fixes the problem for me:

diff --git a/devtools/client/shared/components/reps/reps.js b/devtools/client/shared/components/reps/reps.js
--- a/devtools/client/shared/components/reps/reps.js
+++ b/devtools/client/shared/components/reps/reps.js
@@ -3828,17 +3828,17 @@ const validProtocols = /(http|https|ftp|
 //                       found here: http://www.iana.org/domains/root/db
 //  )
 //  [-\w.!~*'();,/?:@&=+$#%]*
 //                       path onwards. We allow the set of characters that
 //                       encodeURI does not escape plus the result of escaping
 //                       (so also '%')
 // )
 // eslint-disable-next-line max-len
-const urlRegex = /(^|[\s(,;'"`“])((?:https?:\/\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;
+const urlRegex = /(^|[\s(,;'"`“])((?:https?:\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;
 
 // Set of terminators that are likely to have been part of the context rather
 // than part of the URL and so should be uneaten. This is '(', ',', ';', plus
 // quotes and question end-ing punctuation and the potential permutations with
 // parentheses (english-specific).
 const uneatLastUrlCharsRegex = /(?:[),;.!?`'"]|[.!?]\)|\)[.!?])$/;
 
 const ELLIPSIS = "\u2026";

How does that sound? It modifies you PR here:
https://github.com/firefox-devtools/debugger/commit/5f696ab68cd54585b2b106caad7c1431ae4b2cdc
(it changes the regexp)

Honza

Flags: needinfo?(nchevobbe)

This does work, but I feel like it's not very explicit.
Could we have (^|[\s(,;'"“])((?:https?:/(/)?|www\d{0,3}[.][a-z0-9.-]{2,249}|[a-z0-9.-]{2,250}[.][a-z]{2,4}/)[-\w.!~'();,/?:@&=+$#%])` instead? (optional second slash)

Flags: needinfo?(nchevobbe)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)

This does work, but I feel like it's not very explicit.
Could we have (^|[\s(,;'"“])((?:https?:/(/)?|www\d{0,3}[.][a-z0-9.-]{2,249}|[a-z0-9.-]{2,250}[.][a-z]{2,4}/)[-\w.!~'();,/?:@&=+$#%])` instead? (optional second slash)

Yep, looks better.

So, the fix should go to:
devtools\client\debugger\packages\devtools-reps\src\reps\rep-utils.js

And the devtools/client/shared/components/reps/reps.js bundle should be updated

Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f24a38d1b40a
JSONView stopped rendering relative URLs; nchevobbe r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Fix verified with 70.0a1 (2019-08-18) on Windows 10, Ubuntu 16.04, macOS 10.13.

Status: RESOLVED → VERIFIED

Is this something we should consider backporting to Beta for Fx69 or can it ride Fx70 to release?

Flags: needinfo?(odvarko)
Flags: in-testsuite+

Comment on attachment 9084674 [details]
Bug 1566856 - JSONView stopped rendering relative URLs; nchevobbe

Beta/Release Uplift Approval Request

  • User impact if declined: Missing links for http:/ URLs (one slash)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only for developers, just missing link in JSON file preview.
  • String changes made/needed:
Flags: needinfo?(odvarko)
Attachment #9084674 - Flags: approval-mozilla-beta?

Comment on attachment 9084674 [details]
Bug 1566856 - JSONView stopped rendering relative URLs; nchevobbe

Devtools JSONView fix. Approved for 69.0b16.

Attachment #9084674 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.