Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
950433002:
Fix a crash when processing an invalid PAC script. (Closed)
DescriptionFix a crash when processing an invalid PAC script.
BUG=460412
Committed: https://crrev.com/8df9fd8025f73ec3781ed2fb13444bfbdf79899d
Cr-Commit-Position: refs/heads/master@{#318540}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address feedback from mmenke #
Total comments: 6
Patch Set 3 : Address Matt's feedback #
Total comments: 2
Patch Set 4 : Check the error message line numbers as well #Patch Set 5 : Add copyright to test data (placate presubmit) #
Messages
Total messages: 25 (6 generated)
eroman@chromium.org changed reviewers: + jkummerow@chromium.org, mmenke@chromium.org
https://codereview.chromium.org/950433002/diff/1/net/data/proxy_resolver_v8_u... File net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js (right): https://codereview.chromium.org/950433002/diff/1/net/data/proxy_resolver_v8_u... net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js:4: function getterFindProxyForURL() { optional nit: findProxyForURLGetter seems the more common style, at least C++-side. Same for the other file. https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... File net/proxy/proxy_resolver_v8_unittest.cc (right): https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... net/proxy/proxy_resolver_v8_unittest.cc:342: EXPECT_EQ(-1, bindings->errors_line_number[0]); Don't suppose you know if there's a reason this isn't the actual exception we threw?
https://codereview.chromium.org/950433002/diff/1/net/data/proxy_resolver_v8_u... File net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js (right): https://codereview.chromium.org/950433002/diff/1/net/data/proxy_resolver_v8_u... net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js:4: function getterFindProxyForURL() { On 2015/02/20 20:57:49, mmenke wrote: > optional nit: findProxyForURLGetter seems the more common style, at least > C++-side. Same for the other file. Done. https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... File net/proxy/proxy_resolver_v8_unittest.cc (right): https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... net/proxy/proxy_resolver_v8_unittest.cc:342: EXPECT_EQ(-1, bindings->errors_line_number[0]); On 2015/02/20 20:57:49, mmenke wrote: > Don't suppose you know if there's a reason this isn't the actual exception we > threw? Done. I rewired things so the original exception thrown by the access is printed to the error log too. I also tweaked the error messages so we can distinguish between the two scenarios. Thanks for pointing this out! (Sick and WFH today, and I apparently a bit sloppy :-)
Not going to be able to give the changes a full review today. I'll do it first thing Monday. https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... File net/proxy/proxy_resolver_v8_unittest.cc (right): https://codereview.chromium.org/950433002/diff/1/net/proxy/proxy_resolver_v8_... net/proxy/proxy_resolver_v8_unittest.cc:342: EXPECT_EQ(-1, bindings->errors_line_number[0]); On 2015/02/20 22:05:45, eroman wrote: > On 2015/02/20 20:57:49, mmenke wrote: > > Don't suppose you know if there's a reason this isn't the actual exception we > > threw? > > Done. > > I rewired things so the original exception thrown by the access is printed to > the error log too. I also tweaked the error messages so we can distinguish > between the two scenarios. > > Thanks for pointing this out! (Sick and WFH today, and I apparently a bit sloppy > :-) No problem! Sorry to hear you're sick, hope you feel better soon! https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... File net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js (right): https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js:4: function FindProxyForURLGetter() { nit: f should be lowercase, I believe. https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... File net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_resolve.js (right): https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_resolve.js:10: function FindProxyForURLGetter() { nit: f should be lowercase, I believe.
https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... File net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js (right): https://codereview.chromium.org/950433002/diff/20001/net/data/proxy_resolver_... net/data/proxy_resolver_v8_unittest/exception_findproxyforurl_during_init.js:4: function FindProxyForURLGetter() { On 2015/02/20 22:15:05, mmenke wrote: > nit: f should be lowercase, I believe. In my mind "FindProxyForURL" is the target object, so appending "Getter" should not modify the first letter. That said I am fine applying either change!
ping
LGTM, but I'm not an OWNER. From a V8 point of view you're free to land this whenever you're ready. The fixed re-land on our side was https://chromium.googlesource.com/v8/v8/+/dafd73016c3b53fbe87cc67e62b38d2936a... (landed in V8 an hour ago, should roll into Chromium later today, and will be in the next Canary after that, probably 2315).
https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... File net/proxy/proxy_resolver_v8.cc (right): https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... net/proxy/proxy_resolver_v8.cc:530: base::ASCIIToUTF16("Accessing FindProxyForURL threw an exception.")); Is it possible for try_catch.HasCaught() != function->IsEmpty()? It seems weird to have code to catch a thrown errors in a variable...and then use a completely different check to see if we actually caught something. If it is possible, should have a test. If not, should fix this code - either just check one variable and maybe DCHECK on the other, or make the shared error message more general.
https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... File net/proxy/proxy_resolver_v8.cc (right): https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... net/proxy/proxy_resolver_v8.cc:530: base::ASCIIToUTF16("Accessing FindProxyForURL threw an exception.")); On 2015/02/25 17:50:43, mmenke wrote: > Is it possible for try_catch.HasCaught() != function->IsEmpty()? It seems weird > to have code to catch a thrown errors in a variable...and then use a completely > different check to see if we actually caught something. > > If it is possible, should have a test. If not, should fix this code - either > just check one variable and maybe DCHECK on the other, or make the shared error > message more general. I do not know if it is possible for function->IsEmpty() when !try_catch.HasCaught(). Perhaps Jakob can answer that. I think it is reasonable though to test them individually as an extra precaution: - The TryCatch::HasCaught() to print the exception error messsage - The function->IsEmpty() used to avoid null dereference
https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... File net/proxy/proxy_resolver_v8.cc (right): https://codereview.chromium.org/950433002/diff/20001/net/proxy/proxy_resolver... net/proxy/proxy_resolver_v8.cc:530: base::ASCIIToUTF16("Accessing FindProxyForURL threw an exception.")); On 2015/02/25 18:07:26, eroman wrote: > On 2015/02/25 17:50:43, mmenke wrote: > > Is it possible for try_catch.HasCaught() != function->IsEmpty()? It seems > weird > > to have code to catch a thrown errors in a variable...and then use a > completely > > different check to see if we actually caught something. > > > > If it is possible, should have a test. If not, should fix this code - either > > just check one variable and maybe DCHECK on the other, or make the shared > error > > message more general. > > I do not know if it is possible for function->IsEmpty() when > !try_catch.HasCaught(). Perhaps Jakob can answer that. > > I think it is reasonable though to test them individually as an extra > precaution: > - The TryCatch::HasCaught() to print the exception error messsage > - The function->IsEmpty() used to avoid null dereference I'm fine with handling them both, but this code still seems weird. Suggest either: if (HasCaught() || function->IsEmpty()) { if (HasCaught()) .. .. return ERR_PAC_SCRIPT_FAILED; } or if (HasCaught()) { ... return ERR_PAC_SCRIPT_FAILED; } if (IsEmpty()) { // Log error that does *not* claim an exception was thrown. // "Unable to get FindProxyForURL"? return ERR_PAC_SCRIPT_FAILED; }
On 2015/02/25 18:07:26, eroman wrote: > I do not know if it is possible for function->IsEmpty() when > !try_catch.HasCaught(). Perhaps Jakob can answer that. I don't think that can happen. If it does happen, please file a bug :-)
@mmenke: I have added a DCHECK() as well as added a test matching your suggestion.
LGTM https://codereview.chromium.org/950433002/diff/40001/net/proxy/proxy_resolver... File net/proxy/proxy_resolver_v8_unittest.cc (right): https://codereview.chromium.org/950433002/diff/40001/net/proxy/proxy_resolver... net/proxy/proxy_resolver_v8_unittest.cc:347: EXPECT_EQ("Uncaught crash!", bindings->errors[0]); Check line number, here and in the next test?
https://codereview.chromium.org/950433002/diff/40001/net/proxy/proxy_resolver... File net/proxy/proxy_resolver_v8_unittest.cc (right): https://codereview.chromium.org/950433002/diff/40001/net/proxy/proxy_resolver... net/proxy/proxy_resolver_v8_unittest.cc:347: EXPECT_EQ("Uncaught crash!", bindings->errors[0]); On 2015/02/27 20:35:10, mmenke wrote: > Check line number, here and in the next test? Done.
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/950433002/#ps60001 (title: "Check the error message line numbers as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950433002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/950433002/#ps80001 (title: "Add copyright to test data (placate presubmit)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950433002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8df9fd8025f73ec3781ed2fb13444bfbdf79899d Cr-Commit-Position: refs/heads/master@{#318540} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
