[Chromium] Allow pushState and related history APIs to be disabled per context
Created attachment 169515 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 169515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169515&action=review > Source/WebCore/page/History.idl:46 > - [Custom] void replaceState(in any data, in DOMString title, in [Optional] DOMString url) > + [Custom, V8EnabledPerContext=pushState] void replaceState(in any data, in DOMString title, in [Optional] DOMString url) I presume this works even though these are custom.
(In reply to comment #3) > I presume this works even though these are custom. Seems to work.
Comment on attachment 169515 [details] Patch Rejecting attachment 169515 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14455310
Comment on attachment 169515 [details] Patch Rejecting attachment 169515 [details] from commit-queue. New failing tests: fast/loader/stateobjects/popstate-after-load-complete-addeventlistener.html fast/frames/sandboxed-iframe-history-denied.html fast/loader/stateobjects/popstate-after-load-complete-window-attribute.html fast/dom/Window/window-appendages-cleared.html http/tests/navigation/pushstate-updates-referrer.html http/tests/navigation/replacestate-updates-referrer.html http/tests/history/popstate-fires-with-pending-requests.html http/tests/navigation/replacestate-base-illegal.html fast/loader/scroll-position-restored-on-back-non-cached.html http/tests/history/replacestate-post-to-get.html fast/loader/scroll-position-restored-on-back.html fast/loader/text-document-wrapping.html fast/loader/stateobjects/popstate-after-load-complete-body-attribute.html http/tests/navigation/replacestate-base-legal.html fast/loader/stateobjects/document-destroyed-navigate-back.html http/tests/navigation/redirect-on-back-updates-history-item.html http/tests/navigation/redirect-on-reload-updates-history-item.html http/tests/loading/state-object-security-exception.html fast/loader/stateobjects/popstate-after-load-complete-body-inline-attribute.html http/tests/history/replacestate-post-to-get-2.html http/tests/security/cross-frame-access-history-prototype.html fast/loader/document-with-fragment-url-3.html http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html http/tests/history/history-navigations-set-referrer.html fast/history/same-document-iframes-changing-pushstate.html fast/history/history-replace-updates-current-item.html fast/loader/javascript-url-in-object.html fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html fast/loader/document-with-fragment-url-4.html http/tests/security/cross-frame-access-object-getPrototypeOf.html Full output: http://queues.webkit.org/results/14464226
Huh, it looks like V8History::installPerContextPrototypeProperties is called, context is null, so the if context && context->isDocument() && ContextFeatures::pushStateEnabled(static_cast<Document*>(context)) check fails. Looking more into it.
Morita-san may know more, since it looks like he touched this in http://trac.webkit.org/changeset/131167 (see comment 7).
Ah, it's because V8History::wrapSlow ends up with: v8::Handle<v8::Object> V8History::wrapSlow(PassRefPtr<History> impl, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate) Document* document = 0; ... wrapper = V8DOMWrapper::instantiateV8Object(document, &info, impl.get()); So the context is null. That code gets generated by http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm&exact_package=chromium&q=wrapSlow%20file:pm&type=cs&l=3434: push(@implContent, <<END); Document* document = 0; UNUSED_PARAM(document); END if (IsNodeSubType($dataNode) || $interfaceName eq "NotificationCenter") { push(@implContent, <<END); document = impl->document(); END Adding History next to the NotificationCenter exception seems like the most expedient fix. Not sure if there's anything cleaner.
Created attachment 169685 [details] Patch
Comment on attachment 169685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169685&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3438 > - if (IsNodeSubType($dataNode) || $interfaceName eq "NotificationCenter") { > + if (IsNodeSubType($dataNode) || $interfaceName eq "NotificationCenter" || $interfaceName eq "History") { Please don't do this. We're getting rid of this argument. I'm just waiting for the v8-team to make an API change.
Comment on attachment 169685 [details] Patch instantiateV8Object doesn't need to look at its Document parameter. You can get the current document by calling currentDocument from http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/BindingState.h
(In reply to comment #12) > (From update of attachment 169685 [details]) > instantiateV8Object doesn't need to look at its Document parameter. You can get the current document by calling currentDocument from http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/BindingState.h Adam is removing Document parameter with http://webkit.org/b/99876
Comment on attachment 169685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169685&action=review > Source/WebCore/dom/ContextFeatures.cpp:114 > + ASSERT(document); > + if (!document) > + return true; We shouldn't both ASSERT and handle the case where the ASSERT fails. If the ASSERT is legit, we'd prefer to crash and know about the problem.
Created attachment 169758 [details] Patch
> Adam is removing Document parameter with http://webkit.org/b/99876 I took the liberty of updating your patch now that Bug 99876 has landed. Thanks for being patient.
Comment on attachment 169758 [details] Patch Attachment 169758 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14460795
Comment on attachment 169758 [details] Patch Attachment 169758 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14460796
Comment on attachment 169758 [details] Patch Hum... Seems like we need the document member function after all.
(In reply to comment #19) > (From update of attachment 169758 [details]) > Hum... Seems like we need the document member function after all. Hope bug 99954 helps.
Comment on attachment 169758 [details] Patch Ok. Let see if the commit-queue likes it.
(In reply to comment #16) > > Adam is removing Document parameter with http://webkit.org/b/99876 > > I took the liberty of updating your patch now that Bug 99876 has landed. Thanks for being patient. Thanks!
Comment on attachment 169758 [details] Patch Rejecting attachment 169758 [details] from commit-queue. New failing tests: http/tests/security/cross-frame-access-history-prototype.html fast/frames/sandboxed-iframe-history-denied.html http/tests/security/cross-frame-access-object-getPrototypeOf.html Full output: http://queues.webkit.org/results/14490597
(In reply to comment #23) > New failing tests: > http/tests/security/cross-frame-access-history-prototype.html > fast/frames/sandboxed-iframe-history-denied.html > http/tests/security/cross-frame-access-object-getPrototypeOf.html These tests are failing because of additional output lines like: CONSOLE MESSAGE: Sandbox access violation: Unsafe JavaScript attempt to access frame with URL script>. The frame requesting access is sandboxed into a unique origin. It seems like adding V8EnabledPerContext=pushState to the state attribute causes cross-frame history object to print that out (adding it to the pushState() and replaceState() methods doesn't do it). I'll look at the generated code and see if I can puzzle out why that happes.
The stack trace for why we we end up printing that message: #0 WebCore::DOMWindow::crossDomainAccessErrorMessage (this=0x7fffe4eab380, activeWindow=0x7fffe24a3000) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:1756 #1 0x00007ffff192801f in WebCore::reportUnsafeJavaScriptAccess (host=..., type=v8::ACCESS_SET, data=...) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:139 #2 0x00007ffff7347292 in v8::internal::Isolate::ReportFailedAccessCheck (this=0x7fffe4e67000, receiver=0x3541a8fae041, type=v8::ACCESS_SET) at ../../v8/src/isolate.cc:793 #3 0x00007ffff73b6dd9 in v8::internal::JSObject::DefineAccessor (this=0x3541a8fae041, info=0x33a29c027a31) at ../../v8/src/objects.cc:4737 #4 0x00007ffff72a280a in v8::internal::SetAccessor (obj=..., info=...) at ../../v8/src/handles.cc:343 #5 0x00007ffff71c9cc7 in v8::Object::SetAccessor (this=0x7fffe24201b0, name=..., getter= 0x7ffff1970d3e <WebCore::V8History::stateAccessorGetter(v8::Local<v8::String>, v8::AccessorInfo const&)>, setter=0x0, data=..., settings=v8::DEFAULT, attributes=v8::None) at ../../v8/src/api.cc:3119 #6 0x00007ffff21e73d2 in WebCore::V8DOMConfiguration::configureAttribute<v8::Object> (instance=..., prototype=..., attribute=...) at ../../third_party/WebKit/Source/WebCore/bindings/v8/V8DOMConfiguration.h:66 #7 0x00007ffff21c822e in WebCore::V8History::installPerContextProperties (instance=..., impl=0x7fffe13e6a50) at gen/webcore/bindings/V8History.cpp:237 #8 0x00007ffff21c8699 in WebCore::V8History::wrapSlow (impl=..., creationContext=..., isolate=0x7fffe4e67000) at gen/webcore/bindings/V8History.cpp:279 #9 0x00007ffff197bba2 in WebCore::V8History::wrap (impl=0x7fffe13e6a50, creationContext=..., isolate=0x7fffe4e67000) at gen/webkit/bindings/V8History.h:70 #10 0x00007ffff197bbfb in WebCore::toV8 (impl=0x7fffe13e6a50, creationContext=..., isolate=0x7fffe4e67000) at gen/webkit/bindings/V8History.h:77 #11 0x00007ffff2289b86 in WebCore::DOMWindowV8Internal::historyAttrGetter (name=..., info=...) at gen/webcore/bindings/V8DOMWindow.cpp:593 #12 0x00007ffff73a391c in v8::internal::JSObject::GetPropertyWithCallback (this=0x3541a8f3f161, receiver=0x15d8d4b57581, structure=0x15d8d4b62ea9, name=0x3db80c81d0d1) at ../../v8/src/objects.cc:207 #13 0x00007ffff73a429d in v8::internal::JSObject::GetPropertyWithFailedAccessCheck (this=0x15d8d4b57581, receiver=0x15d8d4b57581, result=0x7fffffffb5e0, name=0x3db80c81d0d1, attributes=0x7fffffffb66c) at ../../v8/src/objects.cc:326 #14 0x00007ffff73a577b in v8::internal::Object::GetProperty (this=0x15d8d4b57581, receiver=0x15d8d4b57581, result=0x7fffffffb5e0, name=0x3db80c81d0d1, attributes=0x7fffffffb66c) at ../../v8/src/objects.cc:622 #15 0x00007ffff732e407 in v8::internal::LoadIC::Load (this=0x7fffffffb6b0, state=v8::internal::UNINITIALIZED, object=..., name=...) at ../../v8/src/ic.cc:934 #16 0x00007ffff733389b in v8::internal::LoadIC_Miss (args=..., isolate=0x7fffe4e67000) at ../../v8/src/ic.cc:2088 We did the access check before (we also ended up in V8DOMConfiguration::configureAttribute), but I'm guessing that because we did those on the template meant that we didn't redo it for each context, so we didn't run this code while in a sandboxed iframe (which can't access that property). One option for suppressing this message is to temporarily do a v8::V8::SetFailedAccessCheckCallbackFunction(NULL) when defining the per-context properties. However, that seems hacky (and restoring it to reportUnsafeJavaScriptAccess would mean exposing that from V8DOMWindowShell.cpp, where it's currently a static). I'm going to remove the V8EnabledPerContext=pushState from the state attribute for now, since then the CL works as expected and can therefore be landed. I'm guessing that all feature detection for this API looks for the pushState method, not the state property.
Created attachment 170494 [details] Patch for landing
Comment on attachment 170494 [details] Patch for landing Clearing flags on attachment: 170494 Committed r132421: <http://trac.webkit.org/changeset/132421>
All reviewed patches have been landed. Closing bug.