Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196343011:
document.location bindings fix (Closed)
|
Created:
6 years, 9 months ago by Nate Chapin Modified:
6 years, 9 months ago CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
Descriptiondocument.location bindings fix
BUG=352374
R=jochen@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169176
Patch Set 1 #Patch Set 2 : + bindings update #
Total comments: 4
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... Source/bindings/templates/attributes.cpp:247: {{attribute.v8_value_to_local_cpp_value}}; alternatively, we could just move this up, no?
LGTM
On 2014/03/13 23:26:48, jochen wrote: > https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... > File Source/bindings/templates/attributes.cpp (right): > > https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... > Source/bindings/templates/attributes.cpp:247: > {{attribute.v8_value_to_local_cpp_value}}; > alternatively, we could just move this up, no? Quite possibly, but it's been a while since I've touched the bindings and didn't trust myself to do anything more complicated :)
oh well, it's certainly safe to use a RefPtr<> so lgtm for merging back, you'll have to create a new patch against the old perl generator.
On 2014/03/13 23:33:26, jochen wrote: > oh well, it's certainly safe to use a RefPtr<> so lgtm > > for merging back, you'll have to create a new patch against the old perl > generator. Yep, inferno's working on it right now.
Message was sent while issue was closed.
Committed patchset #2 manually as r169176 (presubmit successful).
Message was sent while issue was closed.
Just a follow-up: - The problem happens in places where C++ raw pointers are initialized from other than info.Holder(). (It is safe to initialize C++ raw pointers from info.Holder() because it's guaranteed that info.Holder() is kept alive in the V8 side.) - As far as I scan the code, line 235 in attributes.cpp is the only place that causes the problem. - The change to line 235 won't affect performance, because it's just used in V8Document, V8HTMLLinkElement, V8HTMLOutputElement and V8Window.
Message was sent while issue was closed.
I have audited all calls to ToString and *V8STRINGRESOURCE* macros in bindings/custom to make sure we aren't holding any raw pointers across toString calls which are not otherwise retained by V8 (info.holder(), info[n] args, info.isolate(), etc.).
Message was sent while issue was closed.
On 2014/03/13 23:50:16, eseidel wrote: > I have audited all calls to ToString and *V8STRINGRESOURCE* macros in > bindings/custom to make sure we aren't holding any raw pointers across toString > calls which are not otherwise retained by V8 (info.holder(), info[n] args, > info.isolate(), etc.). I did too and confirmed that other places would be safe. Thanks guys for the amazingly quick fix!
Message was sent while issue was closed.
On 2014/03/13 23:29:34, Nate Chapin wrote: > https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... > > Source/bindings/templates/attributes.cpp:247: > > {{attribute.v8_value_to_local_cpp_value}}; > > alternatively, we could just move this up, no? > > Quite possibly, but it's been a while since I've touched the bindings and didn't > trust myself to do anything more complicated :) Now that the rewrite is complete, I'm in the middle of cleaning up the compiler, doing things exactly like this (rearranging the templates etc.). Feel free to jump in and rearrange it if you'd like, otherwise I'll get to it in a week or two.
Message was sent while issue was closed.
Question for Jochen (and Nate); we could rearrange the code if that would catch errors earlier and avoid this kind of problem in future. https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... Source/bindings/templates/attributes.cpp:247: {{attribute.v8_value_to_local_cpp_value}}; On 2014/03/13 23:26:49, jochen wrote: > alternatively, we could just move this up, no? How would this help? (Honest question; might be a good change.) I.e., would it be good to do the local variable assignment *before* we get impl? AFAICT the problem was in the assignment to |imp| (renamed to |impl|), right? {{v8_value_to_local_cpp_value}} assigns to a local variable whose type depends on the attribute (in this case the target attribute of the [PutForwards]). In this case Document.location (type Location) forwards to Location.href (type DOMString) ...so the line it generates is: V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue, jsValue); Would putting this before the |impl| setting catch errors earlier? (e.g., on invalid/empty input)
Message was sent while issue was closed.
https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... Source/bindings/templates/attributes.cpp:247: {{attribute.v8_value_to_local_cpp_value}}; On 2014/03/20 07:20:29, Nils Barth wrote: > On 2014/03/13 23:26:49, jochen wrote: > > alternatively, we could just move this up, no? > > How would this help? > (Honest question; might be a good change.) > > I.e., would it be good to do the local variable assignment > *before* we get impl? > > AFAICT the problem was in the assignment to |imp| (renamed to |impl|), right? > > {{v8_value_to_local_cpp_value}} assigns to a local variable > whose type depends on the attribute (in this case > the target attribute of the [PutForwards]). > In this case Document.location (type Location) > forwards to Location.href (type DOMString) > ...so the line it generates is: > V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue, jsValue); this executes arbitrary javascript. Local variables assigned before aren't guaranteed to be valid anymore afterwards. > > Would putting this before the |impl| setting catch errors earlier? > (e.g., on invalid/empty input)
Message was sent while issue was closed.
https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... Source/bindings/templates/attributes.cpp:247: {{attribute.v8_value_to_local_cpp_value}}; On 2014/03/20 07:42:13, jochen wrote: > On 2014/03/20 07:20:29, Nils Barth wrote: > > On 2014/03/13 23:26:49, jochen wrote: > > > alternatively, we could just move this up, no? > > > > How would this help? > > (Honest question; might be a good change.) > > > > I.e., would it be good to do the local variable assignment > > *before* we get impl? > > > > AFAICT the problem was in the assignment to |imp| (renamed to |impl|), right? > > > > {{v8_value_to_local_cpp_value}} assigns to a local variable > > whose type depends on the attribute (in this case > > the target attribute of the [PutForwards]). > > In this case Document.location (type Location) > > forwards to Location.href (type DOMString) > > ...so the line it generates is: > > V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue, jsValue); > > this executes arbitrary javascript. Local variables assigned before aren't > guaranteed to be valid anymore afterwards. Ok, so rearranging the generated code wouldn't help, right?
Message was sent while issue was closed.
On 2014/03/24 05:06:08, Nils Barth wrote: > https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... > File Source/bindings/templates/attributes.cpp (right): > > https://codereview.chromium.org/196343011/diff/20001/Source/bindings/template... > Source/bindings/templates/attributes.cpp:247: > {{attribute.v8_value_to_local_cpp_value}}; > On 2014/03/20 07:42:13, jochen wrote: > > On 2014/03/20 07:20:29, Nils Barth wrote: > > > On 2014/03/13 23:26:49, jochen wrote: > > > > alternatively, we could just move this up, no? > > > > > > How would this help? > > > (Honest question; might be a good change.) > > > > > > I.e., would it be good to do the local variable assignment > > > *before* we get impl? > > > > > > AFAICT the problem was in the assignment to |imp| (renamed to |impl|), > right? > > > > > > {{v8_value_to_local_cpp_value}} assigns to a local variable > > > whose type depends on the attribute (in this case > > > the target attribute of the [PutForwards]). > > > In this case Document.location (type Location) > > > forwards to Location.href (type DOMString) > > > ...so the line it generates is: > > > V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue, jsValue); > > > > this executes arbitrary javascript. Local variables assigned before aren't > > guaranteed to be valid anymore afterwards. > > Ok, so rearranging the generated code wouldn't help, right? If we move all JavaScript execution before assigning raw pointers, we're safe |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
