Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
368643002:
DevTools: Make addFileForURL more robust (Closed)
|
Created:
6 years, 5 months ago by alph Modified:
6 years, 5 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionDevTools: Make addFileForURL more robust
Eliminate the error:
"Uncaught URIError: URI malformed", source: chrome-devtools://devtools/bundled/sdk/NetworkWorkspaceBinding.js (133)
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177449
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added a test. #
Total comments: 5
Patch Set 3 : Compacting the malformed URL #
Total comments: 5
Patch Set 4 : Addressing comments. #
Messages
Total messages: 24 (0 generated)
A test?
https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = decodeURIComponent(parentPath); FYI, there are decodeURI(), decodeURIComponent() and unescape(). Are you sure you use the correct one?
Could you please provide an example URL that causes the problem? Otherwise it's unclear what you are fixing. On 2014/07/01 18:21:52, aandrey wrote: > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): > > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = > decodeURIComponent(parentPath); > FYI, there are decodeURI(), decodeURIComponent() and unescape(). > Are you sure you use the correct one? unescape is deprecated
On 2014/07/02 08:59:22, vsevik wrote: > Could you please provide an example URL that causes the problem? Otherwise it's > unclear what you are fixing. It is in the test.
ptal https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = decodeURIComponent(parentPath); On 2014/07/01 18:21:52, aandrey wrote: > FYI, there are decodeURI(), decodeURIComponent() and unescape(). > Are you sure you use the correct one? Thanks, I know about these. decodeURI is supposed to be called on complete URIs. unescape is deprecated. decodeURIComponent seems to be the one to use here. Though ideally it should be called on each component before join, but it works this way as well. Do you have a specific concern?
On 2014/07/02 09:49:22, alph wrote: > ptal > > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): > > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = > decodeURIComponent(parentPath); > On 2014/07/01 18:21:52, aandrey wrote: > > FYI, there are decodeURI(), decodeURIComponent() and unescape(). > > Are you sure you use the correct one? > > Thanks, I know about these. > decodeURI is supposed to be called on complete URIs. > unescape is deprecated. > decodeURIComponent seems to be the one to use here. Though ideally it should be > called on each component before join, but it works this way as well. > > Do you have a specific concern? Calling decodeURIComponent without the corresponding encodeURIComponent on our side seems buggy to me (that why it can throw). But maybe it's the only right thing to do here.
On 2014/07/02 10:15:32, aandrey wrote: > On 2014/07/02 09:49:22, alph wrote: > > ptal > > > > > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > > File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): > > > > > https://codereview.chromium.org/368643002/diff/1/Source/devtools/front_end/sd... > > Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = > > decodeURIComponent(parentPath); > > On 2014/07/01 18:21:52, aandrey wrote: > > > FYI, there are decodeURI(), decodeURIComponent() and unescape(). > > > Are you sure you use the correct one? > > > > Thanks, I know about these. > > decodeURI is supposed to be called on complete URIs. > > unescape is deprecated. > > decodeURIComponent seems to be the one to use here. Though ideally it should > be > > called on each component before join, but it works this way as well. > > > > Do you have a specific concern? > > Calling decodeURIComponent without the corresponding encodeURIComponent on our > side seems buggy to me (that why it can throw). > But maybe it's the only right thing to do here. AFAICT the decodeURI has been used to prettify the file names we derive from the URL. See https://codereview.chromium.org/349523003
https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/resource-script-mapping.html (right): https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/resource-script-mapping.html:210: var url = "http://vk.com/widget_like.php?app=2925955&width=100%&_ver=1&page=0&url=http%3A%2F%2Flenta.ru%2Fnews%2F2014%2F06%2F23%2Fnavalny%2F&type=button&verb=1&color=&title=%D0%A0%D0%B5%D0%BF%D1%80%D0%BE%D0%B4%D1%83%D0%BA%D1%86%D0%B8%D1%8E%20%D0%B8%D0%B7%D1%8A%D1%8F%D1%82%D0%BE%D0%B9%20%D1%83%20%D0%9D%D0%B0%D0%B2%D0%B0%D0%BB%D1%8C%D0%BD%D0%BE%D0%B3%D0%BE%20%D0%BA%D0%B0%D1%80%D1%82%D0%B8%D0%BD%D1%8B%20%D0%BF%D0%BE%D0%B2%D0%B5%D1%81%D0%B8%D0%BB%D0%B8%20%D0%BD%D0%B0%20%D0%9A%D1%83%D1%82%D1%83%D0%B7%D0%BE%D0%B2%D1%81%D0%BA%D0%BE%D0%BC%20%D0%BF%D1%80%D0%BE%D1%81%D0%BF%D0%B5%D0%BA%D1%82%D0%B5&image=http%3A%2F%2Ficdn.lenta.ru%2Fimages%2F2014%2F06%2F23%2F16%2F20140623161555888%2Fpic_83fbbd49ee3e37c37083bda0c8f0f051.jpg&text=&h=20&height=20&146f6515efe"; decodeURIComponent("http://vk.com/widget_like.php?app=2925955&width=100%&_ver=1") throws. "%" char is the troublesome one
https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/resource-script-mapping.html (right): https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/resource-script-mapping.html:208: function testMalformedResourceURI(next) This belongs to LayoutTests/inspector/sources/debugger/navigator-view.html https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/resource-script-mapping.html:210: var url = "http://vk.com/widget_like.php?app=2925955&width=100%&_ver=1&page=0&url=http%3A%2F%2Flenta.ru%2Fnews%2F2014%2F06%2F23%2Fnavalny%2F&type=button&verb=1&color=&title=%D0%A0%D0%B5%D0%BF%D1%80%D0%BE%D0%B4%D1%83%D0%BA%D1%86%D0%B8%D1%8E%20%D0%B8%D0%B7%D1%8A%D1%8F%D1%82%D0%BE%D0%B9%20%D1%83%20%D0%9D%D0%B0%D0%B2%D0%B0%D0%BB%D1%8C%D0%BD%D0%BE%D0%B3%D0%BE%20%D0%BA%D0%B0%D1%80%D1%82%D0%B8%D0%BD%D1%8B%20%D0%BF%D0%BE%D0%B2%D0%B5%D1%81%D0%B8%D0%BB%D0%B8%20%D0%BD%D0%B0%20%D0%9A%D1%83%D1%82%D1%83%D0%B7%D0%BE%D0%B2%D1%81%D0%BA%D0%BE%D0%BC%20%D0%BF%D1%80%D0%BE%D1%81%D0%BF%D0%B5%D0%BA%D1%82%D0%B5&image=http%3A%2F%2Ficdn.lenta.ru%2Fimages%2F2014%2F06%2F23%2F16%2F20140623161555888%2Fpic_83fbbd49ee3e37c37083bda0c8f0f051.jpg&text=&h=20&height=20&146f6515efe"; Please use a reduced version of this URL, e.g. http://example.com/page.php?%
ptal https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/resource-script-mapping.html (right): https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/resource-script-mapping.html:208: function testMalformedResourceURI(next) On 2014/07/02 12:01:32, vsevik wrote: > This belongs to LayoutTests/inspector/sources/debugger/navigator-view.html Done. https://codereview.chromium.org/368643002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/resource-script-mapping.html:210: var url = "http://vk.com/widget_like.php?app=2925955&width=100%&_ver=1&page=0&url=http%3A%2F%2Flenta.ru%2Fnews%2F2014%2F06%2F23%2Fnavalny%2F&type=button&verb=1&color=&title=%D0%A0%D0%B5%D0%BF%D1%80%D0%BE%D0%B4%D1%83%D0%BA%D1%86%D0%B8%D1%8E%20%D0%B8%D0%B7%D1%8A%D1%8F%D1%82%D0%BE%D0%B9%20%D1%83%20%D0%9D%D0%B0%D0%B2%D0%B0%D0%BB%D1%8C%D0%BD%D0%BE%D0%B3%D0%BE%20%D0%BA%D0%B0%D1%80%D1%82%D0%B8%D0%BD%D1%8B%20%D0%BF%D0%BE%D0%B2%D0%B5%D1%81%D0%B8%D0%BB%D0%B8%20%D0%BD%D0%B0%20%D0%9A%D1%83%D1%82%D1%83%D0%B7%D0%BE%D0%B2%D1%81%D0%BA%D0%BE%D0%BC%20%D0%BF%D1%80%D0%BE%D1%81%D0%BF%D0%B5%D0%BA%D1%82%D0%B5&image=http%3A%2F%2Ficdn.lenta.ru%2Fimages%2F2014%2F06%2F23%2F16%2F20140623161555888%2Fpic_83fbbd49ee3e37c37083bda0c8f0f051.jpg&text=&h=20&height=20&146f6515efe"; On 2014/07/02 12:01:32, vsevik wrote: > Please use a reduced version of this URL, e.g. http://example.com/page.php?%25 Sorry, I just didn't want to invest in finding what's wrong with the URL. ;-) Done.
https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/navigator-view.html (right): https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/navigator-view.html:69: addUISourceCode("%malformedURI", false); could you also test a real world example, like: rootURL + "foo/bar/contentScript.js?a=10%
https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/navigator-view.html (right): https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/navigator-view.html:69: addUISourceCode("%malformedURI", false); I think we only need one URL for this test: "http://example.com/foo?bar=100%&baz=a%20b". This will emphasizes the fact that decodeURI doesn't work for this case. https://codereview.chromium.org/368643002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): https://codereview.chromium.org/368643002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = decodeURIComponent(parentPath); We should use decodeURI here because of the difference in %2f treatment in this two functions. We should never allow decoding %2f to a slash symbol here since it will affect the way we split the URL into the path components.
https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/navigator-view.html (right): https://codereview.chromium.org/368643002/diff/40001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/navigator-view.html:69: addUISourceCode("%malformedURI", false); On 2014/07/02 12:43:40, vsevik wrote: > I think we only need one URL for this test: > "http://example.com/foo?bar=100%&baz=a%20b". > This will emphasizes the fact that decodeURI doesn't work for this case. Done. https://codereview.chromium.org/368643002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js (right): https://codereview.chromium.org/368643002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/sdk/NetworkWorkspaceBinding.js:134: parentPath = decodeURIComponent(parentPath); On 2014/07/02 12:43:40, vsevik wrote: > We should use decodeURI here because of the difference in %2f treatment in this > two functions. We should never allow decoding %2f to a slash symbol here since > it will affect the way we split the URL into the path components. Done.
lgtm
lgtm
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alph@chromium.org/368643002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15082)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15097)
The CQ bit was checked by loislo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alph@chromium.org/368643002/60001
Message was sent while issue was closed.
Change committed as 177449 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
