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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Erik Arvidsson]

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

[Erik Arvidsson]

Created attachment 157341 [details]
Patch

[Adam Barth]

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 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 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]

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

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

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

[Kentaro Hara]

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

Created attachment 157541 [details]
Perf comparison

[Erik Arvidsson]

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 on attachment 157341 [details]
Patch

Thanks. LGTM.

[Erik Arvidsson]

Created attachment 157590 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157590 [details]
Patch for landing

Clearing flags on attachment: 157590

Committed r125232: <http://trac.webkit.org/changeset/125232>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.