Summary: | [V8] Fix issues with [DoNotCheckSecurity] which depended on non ES5 compliant behavior | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||
Component: | New Bugs | Assignee: | Erik Arvidsson <arv> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, haraken, japhet, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 91031 | ||||||||||
Attachments: |
|
Description
Erik Arvidsson
2012-08-08 16:57:29 PDT
Created attachment 157341 [details]
Patch
Comment on attachment 157341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157341&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:800 > + if (!BindingSecurity::shouldAllowAccessToFrame(BindingState::instance(), imp->frame(), ReportSecurityError)) ReportSecurityError is the default. You don't need to supply it explicitly. Comment on attachment 157341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157341&action=review > Source/WebCore/ChangeLog:13 > + No new tests. No change in behavior. Does this fix tests that fail when the ES5 flag is flipped? Comment on attachment 157341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157341&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:777 > + v8::Local<v8::Value> hiddenValue = info.This()->GetHiddenValue(name); GetHiddenValue() and SetHiddenValue() are slow. Isn't there any performance concern? If this code would not be a bottleneck in practical world, that's fine. (In reply to comment #4) > GetHiddenValue() and SetHiddenValue() are slow. Isn't there any performance concern? If this code would not be a bottleneck in practical world, that's fine. They are slow compared to doing nothing but they are not slow slow. I don't have a better idea how to fix this. (In reply to comment #3) > (From update of attachment 157341 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157341&action=review > > > Source/WebCore/ChangeLog:13 > > + No new tests. No change in behavior. > > Does this fix tests that fail when the ES5 flag is flipped? Yes. That is the point of this patch. I can add that to the ChangeLog. Ok. LGTM, but I'd like Kentaro to do the official review, if he's willing. (In reply to comment #5) > They are slow compared to doing nothing but they are not slow slow. I don't have a better idea how to fix this. I think, for correctness, the slowdown is acceptable if the slowdown is not in hot call paths in practical. Would you confirm that this patch doesn't regress performance of PerformanceTests/Dromaeo/ and PerformanceTests/Bindings/? Created attachment 157541 [details]
Perf comparison
The perf results show no regression as far as I can tell. Some tests are faster, some are slower. The results are just really flaky so it is hard to tell. Comment on attachment 157341 [details]
Patch
Thanks. LGTM.
Created attachment 157590 [details]
Patch for landing
Comment on attachment 157590 [details] Patch for landing Clearing flags on attachment: 157590 Committed r125232: <http://trac.webkit.org/changeset/125232> All reviewed patches have been landed. Closing bug. |