Bug 93555

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

91031

Attachments:

Description	Flags
Patch	none
Perf comparison	none
Patch for landing	none

Erik Arvidsson

Reported 2012-08-08 16:57:29 PDT

[V8] Fix issues with [DoNotCheckSecurity] which depended on non ES5 compliant behavior

Attachments
Patch (8.72 KB, patch) 2012-08-08 17:06 PDT, Erik Arvidsson	no flags	Details Formatted Diff Diff
Perf comparison (28.00 KB, text/html) 2012-08-09 13:57 PDT, Erik Arvidsson	no flags	Details
Patch for landing (8.61 KB, patch) 2012-08-09 17:15 PDT, Erik Arvidsson	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Erik Arvidsson

Comment 1 2012-08-08 17:06:34 PDT

Created attachment 157341 [details] Patch

Adam Barth

Comment 2 2012-08-08 17:09:59 PDT

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.

Adam Barth

Comment 3 2012-08-08 17:12:08 PDT

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?

Kentaro Hara

Comment 4 2012-08-08 17:12:43 PDT

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.

Erik Arvidsson

Comment 5 2012-08-08 18:11:33 PDT

(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.

Erik Arvidsson

Comment 6 2012-08-08 18:12:20 PDT

(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.

Adam Barth

Comment 7 2012-08-08 18:17:20 PDT

Ok. LGTM, but I'd like Kentaro to do the official review, if he's willing.

Kentaro Hara

Comment 8 2012-08-08 18:18:45 PDT

(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/?

Erik Arvidsson

Comment 9 2012-08-09 13:57:45 PDT

Created attachment 157541 [details] Perf comparison

Erik Arvidsson

Comment 10 2012-08-09 15:52:14 PDT

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.

Kentaro Hara

Comment 11 2012-08-09 16:36:43 PDT

Comment on attachment 157341 [details] Patch Thanks. LGTM.

Erik Arvidsson

Comment 12 2012-08-09 17:15:45 PDT

Created attachment 157590 [details] Patch for landing

WebKit Review Bot

Comment 13 2012-08-09 18:52:29 PDT

Comment on attachment 157590 [details] Patch for landing Clearing flags on attachment: 157590 Committed r125232: <http://trac.webkit.org/changeset/125232>

WebKit Review Bot

Comment 14 2012-08-09 18:52:33 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.