Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[Affected versions]:
-Firefox 61.0.1, 62.0b10, 63.0a1 (2018-07-23), 52.8.0esr, 60.0.2esr

[Affected platforms]:
- win 10x64, macOS 10.13.4, Ubuntu 16.04LTS

[Steps to reproduce]:
1. Launch Firefox;
2. Open DevTools - Netmonitor;
3. Navigate to any webpage so the list is populated;
4. Right click on any method-request;
5. Click on the Edit and Resend option;
6. Select the whole request link;
7. Type anything instead;

[Expected result]:
- the link is replaced with the input;

[Actual result]:
- the whole devTools content section is blank, header-tabs are unafected;

[Regression range]: mozregression points out to
- https://bugzilla.mozilla.org/show_bug.cgi?id=1416824

[Additional notes]:
- attached screen recording with the issue;
- console error message: "TypeError: v is not a valid URL."  (v is the added text in this case)
- on 60.0a1, before this bug: after selection of whole text, typing anything adds the text at the end of the string, not replacing it;
- the request headers and request body fields are not affected;

	Marion Daly [:mdaly]
	Comment 1 • 6 years ago

Hi Patrick, 

Could you help us with triaging this one?

	Julien Cristau [:jcristau]
	Comment 2 • 6 years ago

The regression range is inconsistent with this affecting 52.

	Patrick Brosset <:pbro>
	Comment 3 • 6 years ago

Oh wow, thanks for filing. This should definitely be a P1 (although a little hard to reach hopefully, so not affecting too many users I hope).
I'll try to get someone to look at this quickly.
This affects all current versions of Firefox, so at the very least we should try to uplift to beta when we find a fix.

	David Walsh :davidwalsh Assignee
	Comment 4 • 6 years ago

Error stack is:

IndexedDB UnknownErr: ActorsParent.cpp:13933

TypeError: "f is not a valid URL."
getUrlQueryresource://devtools/client/netmonitor/src/utils/request-utils.js:176:11
renderresource://devtools/client/netmonitor/src/components/CustomRequestPanel.js:183:47
finishClassComponentresource://devtools/client/shared/vendor/react-dom.js:10332:22
updateClassComponentresource://devtools/client/shared/vendor/react-dom.js:10295:10
beginWorkresource://devtools/client/shared/vendor/react-dom.js:10894:14
performUnitOfWorkresource://devtools/client/shared/vendor/react-dom.js:12713:12
workLoopresource://devtools/client/shared/vendor/react-dom.js:12730:24
renderRootresource://devtools/client/shared/vendor/react-dom.js:12770:7
performWorkOnRootresource://devtools/client/shared/vendor/react-dom.js:13359:22
performWorkresource://devtools/client/shared/vendor/react-dom.js:13281:71
resource://devtools/client/shared/vendor/react-dom.js:13526:5
batchedUpdatesresource://devtools/client/shared/vendor/react-dom.js:2034:7
dispatchEventresource://devtools/client/shared/vendor/react-dom.js:4341:51
resource://devtools/client/shared/vendor/react-dom.js:13513:12
interactiveUpdatesresource://devtools/client/shared/vendor/react-dom.js:2041:10
dispatchInteractiveEventresource://devtools/client/shared/vendor/react-dom.js:4318:3
addEventBubbleListenerresource://devtools/client/shared/vendor/react-dom.js:3745:3
trapBubbledEventresource://devtools/client/shared/vendor/react-dom.js:4292:3
listenToresource://devtools/client/shared/vendor/react-dom.js:4488:13
ensureListeningToresource://devtools/client/shared/vendor/react-dom.js:6087:31
resource://devtools/client/shared/vendor/react-dom.js:6244:7
finalizeInitialChildrenresource://devtools/client/shared/vendor/react-dom.js:6771:3
completeWorkresource://devtools/client/shared/vendor/react-dom.js:11178:17
completeUnitOfWorkresource://devtools/client/shared/vendor/react-dom.js:12591:18
performUnitOfWorkresource://devtools/client/shared/vendor/react-dom.js:12718:12
workLoopresource://devtools/client/shared/vendor/react-dom.js:12730:24
renderRootresource://devtools/client/shared/vendor/react-dom.js:12770:7
performWorkOnRootresource://devtools/client/shared/vendor/react-dom.js:13359:22
performWorkresource://devtools/client/shared/vendor/react-dom.js:13281:7
performSyncWorkresource://devtools/client/shared/vendor/react-dom.js:13253:3
requestWorkresource://devtools/client/shared/vendor/react-dom.js:13153:51
resource://devtools/client/shared/vendor/react-dom.js:13022:11
scheduleRootUpdateresource://devtools/client/shared/vendor/react-dom.js:13566:3
updateContainerAtExpirationTimeresource://devtools/client/shared/vendor/react-dom.js:13581:10
updateContainerresource://devtools/client/shared/vendor/react-dom.js:13608:10
renderresource://devtools/client/shared/vendor/react-dom.js:13853:3
legacyRenderSubtreeIntoContainerresource://devtools/client/shared/vendor/react-dom.js:13965:9
unbatchedUpdatesresource://devtools/client/shared/vendor/react-dom.js:13478:10
legacyRenderSubtreeIntoContainerresource://devtools/client/shared/vendor/react-dom.js:13961:5
renderresource://devtools/client/shared/vendor/react-dom.js:14012:12
bootstrapresource://devtools/client/netmonitor/src/app.js:64:5
openresource://devtools/client/netmonitor/panel.js:24:11
onLoadresource://devtools/client/framework/toolbox.js:1710:21
 react-dom.js:11273:5

TypeError: f is not a valid URL.[Learn More] request-utils.js:176:11

	David Walsh :davidwalsh Assignee
	Comment 5 • 6 years ago

Shooting to you :pbro; please reassign as you see fit or let me know if something is missing.  Thank you!

	David Walsh :davidwalsh Assignee
	Comment 6 • 6 years ago

The root issue here is that using `new URL(unvalided_url_format)` triggers an error, so we need to protect all cases of `new URL()`.

	Patrick Brosset <:pbro>
	Comment 7 • 6 years ago

Comment on attachment 8994952 [details] [diff] [review]
1477915-1.patch

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

Thanks David.
What do you think of this suggestion, I think it might look at bit better/be easier to read:

1. create a helper function like this: getURL
/**
 * Get a URL object from a string url.
 * @param {String} url The input url string
 * @return {Object} Either the URL object, or null if the provided input couldn't be parsed as a valid url
 */
function getURL(url) {
  try {
    return new URL(url);
  } catch (e) {
    return null;
  }
}

2. Then change all the places you changed to something like:

const urlObj = getURL(url);
if (!urlObj) {
  return "";
}
...

This would avoid repeating the try/catch everywhere, and makes all these call sites be more explicit (I feel) about how they handle all sorts of url objects.

R+ because this is only a stylistic suggestion. If you do go with it, no need to ask for a review again.

	David Walsh :davidwalsh Assignee
	Comment 8 • 6 years ago

I made your suggested update and went a step further to condense code and remove repeated `if (!url) return ""`.

Let me know if you think it's too much "magic".  I've also kicked off a try run since `./mach test netmonitor --headless` doesn't seem to finish for me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6820fdf0663e9345bf702a65c610ecdc23f6c59d

Thank you!

	Patrick Brosset <:pbro>
	Comment 9 • 6 years ago

Comment on attachment 8995232 [details] [diff] [review]
1477915-2.patch

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

I like it!

::: devtools/client/netmonitor/src/utils/request-utils.js
@@ +155,5 @@
>  /**
> + * Helpers for retrieving a URL object from a string
> + *
> + * @param {string} url - unvalidated url string
> + * @return {string} unicode query of a url

I think the return value should be documented as such:
@return {URL} The URL object

@@ +166,5 @@
> +  }
> +}
> +
> +/**
> + * Helpers for getting the query portion of a url.

Hm, not just the query portion, this returns any property of the URL object. So maybe rephrase a bit.

	David Walsh :davidwalsh Assignee
	Comment 10 • 6 years ago

Third time is a charm. :)

	Pulsebot
	Comment 11 • 6 years ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/127e7ceb9d87
Prevent netmonitor crash due to invalid URL input r=pbro

	Cristina Coroiu [:ccoroiu]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/127e7ceb9d87

	Cristian Fogel [:cfogel] Reporter
	Comment 13 • 6 years ago

Verified with 63.0a1 (2018-07-30) on macOS 10.9 Ubuntu 16.04LTS, win 10x64.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago

Is this something we could land an automated test for? Also, please request Beta approval on this when you get a chance.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 15 • 6 years ago

Honza, do you want to request uplift here?

	David Walsh :davidwalsh Assignee
	Comment 16 • 6 years ago

:honza Where can we add the test per :RyanVM's request?

I also think we should uplift considering the bug bricks the console; just my 2 cents. :)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 17 • 6 years ago

(In reply to David Walsh :davidwalsh from comment #16)
> :honza Where can we add the test per :RyanVM's request?

Network panel's tests are in this directory: devtools/client/netmonitor/test/

For this we need a new mochitest.

You can take some inspiration from this test:
devtools\client\netmonitor\test\browser_net_resend.js

Or from this one:
devtools\client\netmonitor\test\browser_net_telemetry_edit_resend.js
It's in a patch that is just landing, so you might take a look here (if not landed yet):
https://phabricator.services.mozilla.com/D3238

We should file a new bug report for the test implementation.

> I also think we should uplift considering the bug bricks the console; just
> my 2 cents. :)
Yes, agree.

Thanks David!
Honza

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 18 • 6 years ago

Can you request uplift? This can still land for the beta 18 build.

	David Walsh :davidwalsh Assignee
	Comment 19 • 6 years ago

Comment on attachment 8995284 [details] [diff] [review]
1477915-3.patch

Approval Request Comment
[Feature/Bug causing the regression]: Any change to the widget in question completely bricks the Net Monitor pane when an incomplete URL is provided to the URL textbox
[User impact if declined]: The NetMonitor will brick when users try to type a URL into the custom URL field
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:  Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No -- we're simply doing a try/catch around the new URL usage
[Why is the change risky/not risky?]:
[String changes made/needed]:

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 20 • 6 years ago

Comment on attachment 8995284 [details] [diff] [review]
1477915-3.patch

Fix verified in nightly, let's uplift for beta 18.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 21 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b1e62bab89ae

	Cristian Fogel [:cfogel] Reporter
	Comment 22 • 6 years ago

Fix verified on beta18 over Win10x64, macOS10.9, Ubuntu16.04LTS