Bug 35180 – [V8] V8LocationCustom.cpp doesn't handle location.hash updates properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dirk Pranke]

See http://crbug.com/32633 .

If we update the hash on an "about:blank" URL, the code first decides that we're changing the location and then thinks that we're changing it in a way that triggers a navigation, producing an infinite loop in the following HTML:

<script> 
  function displayPrivacy() {
    alert(trm2.location);
    trm2.location.hash = 'Privacy';
  }
</script>
<iframe name=trm2 id=trm2 onload="displayPrivacy()" 
src="about:blank"></iframe>

[Dirk Pranke]

Created attachment 49238 [details]
tentative patch for this fix - don't review / land, it doesn't have the changelog entry

[Darin Fisher (:fishd, Google)]

This change looks good to me.

[Dirk Pranke]

Created attachment 49462 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 49462 [details]
Patch

> +++ b/LayoutTests/fast/loader/about-blank-hash-change.html
> @@ -0,0 +1,34 @@
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}
> +
> +function onload_callback() {
> +  var old_hash = inner.location.hash;
> +  inner.location.hash = 'hash-ref';
> +  var c = document.getElementById('content');
> +  if (c.innerHTML.match(/^No callback/)) {
> +    c.innerHTML = 'PASS: inner.location.hash = "' + inner.location.hash + '"';
> +  } else if (c.innerHTML.match(/^PASS/)) {
> +    c.innerHTML = c.innerHTML + 'FAIL: inner.location.hash = "' + inner.location.hash + '"';
> +  }

up top you use 4 space indent, but here you use 2 spaces.  it'd be
good to use consistent indentation.


> +++ b/WebCore/bindings/v8/custom/V8LocationCustom.cpp
> @@ -70,15 +70,23 @@ void V8Location::hashAccessorSetter(v8::Local<v8::String> name, v8::Local<v8::Va
>      if (!frame)
>          return;
>  
> -    KURL url = frame->loader()->url();
> +    // We copy the URL to avoid accidentally modifying it in place
> +    // and then returning without actually doing anything. This is
> +    // perhaps overly conservative.
> +    KURL url = frame->loader()->url().copy();

This is unnecessary.  The .copy method is used when you need to copy
a KURL to another thread.  The existing code was using the copy ctor
which is perfect for this single threaded use case.

[Dirk Pranke]

> > +++ b/WebCore/bindings/v8/custom/V8LocationCustom.cpp
> > @@ -70,15 +70,23 @@ void V8Location::hashAccessorSetter(v8::Local<v8::String> name, v8::Local<v8::Va
> >      if (!frame)
> >          return;
> >  
> > -    KURL url = frame->loader()->url();
> > +    // We copy the URL to avoid accidentally modifying it in place
> > +    // and then returning without actually doing anything. This is
> > +    // perhaps overly conservative.
> > +    KURL url = frame->loader()->url().copy();
> 
> This is unnecessary.  The .copy method is used when you need to copy
> a KURL to another thread.  The existing code was using the copy ctor
> which is perfect for this single threaded use case.

I thought from reading the comments in kurl.h that the copy ctor was only doing a shallow copy and so when I updated the .hash in the copy the original would also be updated. I'm wrong?

[Darin Fisher (:fishd, Google)]

Right.  It does a shallow copy of objects that are copy-on-write.  Those objects (String, CString) do not implement threadsafe copy-on-write, so it is not safe to use the copy constructor to create a KURL instance that you can then pass to another thread.

[Dirk Pranke]

Created attachment 49526 [details]
Patch

[Dirk Pranke]

Created attachment 49545 [details]
revised patch - fix js indentation in test file.

[Dirk Pranke]

Created attachment 49546 [details]
whoops - lost the changelogs; reattaching

[WebKit Commit Bot]

Comment on attachment 49546 [details]
whoops - lost the changelogs; reattaching

Clearing flags on attachment: 49546

Committed r55285: <http://trac.webkit.org/changeset/55285>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.