Bug 99780 – [Chromium] Allow pushState and related history APIs to be disabled per context

• Component: History
• Status: RESOLVED FIXED

[Mihai Parparita]

[Chromium] Allow pushState and related history APIs to be disabled per context

[Mihai Parparita]

Created attachment 169515 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Mihai Parparita]

(In reply to comment #3)
> I presume this works even though these are custom.

Seems to work.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mihai Parparita]

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.

[Mihai Parparita]

Morita-san may know more, since it looks like he touched this in http://trac.webkit.org/changeset/131167 (see comment 7).

[Mihai Parparita]

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.

[Mihai Parparita]

Created attachment 169685 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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

[Mihai Parparita]

(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

[Adam Barth]

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.

[Adam Barth]

Created attachment 169758 [details]
Patch

[Adam Barth]

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

[WebKit Review Bot]

Comment on attachment 169758 [details]
Patch

Attachment 169758 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14460795

[Peter Beverloo (cr-android ews)]

Comment on attachment 169758 [details]
Patch

Attachment 169758 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14460796

[Adam Barth]

Comment on attachment 169758 [details]
Patch

Hum...  Seems like we need the document member function after all.

[Hajime Morrita]

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

[Adam Barth]

Comment on attachment 169758 [details]
Patch

Ok.  Let see if the commit-queue likes it.

[Mihai Parparita]

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

[WebKit Review Bot]

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

[Mihai Parparita]

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

[Mihai Parparita]

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.

[Mihai Parparita]

Created attachment 170494 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 170494 [details]
Patch for landing

Clearing flags on attachment: 170494

Committed r132421: <http://trac.webkit.org/changeset/132421>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.