test_shell_webblobregistry_impl.cc posts CString to other threads without

Copying the WebURL that are params to PostTask to copy the underlying WebCString and avoid multithreaded refcounting 

BUG=78208
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82484

[Dmitry Lomov, 2011-04-19 22:17:02]

Copies the underlying WebCString to avoid multithreaded refcounting

[tony, 2011-04-19 22:27:35]

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:41: static void
CopyWebURL(const WebURL& source, WebURL* target) {
I would probably just return a WebURL.  There's an extra copy, but this code
doesn't seem time critical.  *shrug*

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:90: url));
Did you mean to use url_copy here?

[jianli, 2011-04-19 22:53:02]

Thanks for fixing this. Here're some comments.

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:41: static void
CopyWebURL(const WebURL& source, WebURL* target) {
On 2011/04/19 22:27:35, tony wrote:
> I would probably just return a WebURL.  There's an extra copy, but this code
> doesn't seem time critical.  *shrug*

I agree with Tony that returning a WebURL will make the calling code look
better. Also, it might be better to call it as GetUrlCopy.

In addition, can we move this helper function to the anonymouse namespace above.

http://codereview.chromium.org/6878059/diff/2002/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:42: // Creates a copy
of WebCString with refcount 1
Please end with period.

[Dmitry Lomov, 2011-04-19 23:06:00]

Found one more issue: NewRunnableMethod creates one more copy of WebURL, which
creates one more copy of WebCString, which still leads to racy refcounting.
Fixed and addressed other CR feedback

[jianli, 2011-04-19 23:58:00]

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:26: // Copy WebURL
object to safely pass it to a different thread.
Maybe better to add more comment like the following:
// Note: NewRunnableMethod does copy WebURL object, but not its underlying
WebCString object since it is ref-counted. To pass safely to another thread, we
need to make a copy of WebCString data.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:27: static WebURL
GetWebURLCopy(const WebURL& source) {
I think you can remove "static".

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:28: // Creates a copy
of WebCString with refcount 1.
I think this comment can be removed with the additional comment above.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:79: url_copy,
You can put GetWebURLCopy(url) here directly. Doing so will also remove the need
of {}.

Also no need to add "::".

[Dmitry Lomov, 2011-04-20 00:27:00]

Addressed CR feedback. 
Added comments clarifying the need for blocks around calls to NewRunnableMethod.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:26: // Copy WebURL
object to safely pass it to a different thread.
On 2011/04/19 23:58:00, jianli wrote:
> Maybe better to add more comment like the following:
> // Note: NewRunnableMethod does copy WebURL object, but not its underlying
> WebCString object since it is ref-counted. To pass safely to another thread,
we
> need to make a copy of WebCString data.

Done.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:27: static WebURL
GetWebURLCopy(const WebURL& source) {
On 2011/04/19 23:58:00, jianli wrote:
> I think you can remove "static".

Done.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:28: // Creates a copy
of WebCString with refcount 1.
On 2011/04/19 23:58:00, jianli wrote:
> I think this comment can be removed with the additional comment above.

Done.

http://codereview.chromium.org/6878059/diff/6003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:79: url_copy,
On 2011/04/19 23:58:00, jianli wrote:
> You can put GetWebURLCopy(url) here directly. Doing so will also remove the
need
> of {}.
> 
> Also no need to add "::".

Done.

[jianli, 2011-04-20 00:38:02]

LGTM with one small nit.

http://codereview.chromium.org/6878059/diff/3003/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/3003/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:29: // This method
creates as deep copy of WebURL.
as => a

[Dmitry Lomov, 2011-04-20 00:47:11]

Comment misprint fixed

[tony, 2011-04-20 01:01:40]

LGTM

[commit-bot: I haz the power, 2011-04-20 01:08:28]

Presubmit check for 6878059-6 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  webkit/tools/test_shell/test_shell_webblobregistry_impl.cc

[Dmitry Lomov, 2011-04-20 18:04:35]

Copyright header changed - could someone push the commit queue button again?
Thanks!

Mitya

[commit-bot: I haz the power, 2011-04-20 18:37:07]

Try job failure for 6878059-7 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-04-20 21:24:29]

Try job failure for 6878059-7 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-04-21 14:37:18]

Change committed as 82484

[levin, 2011-04-21 15:41:00]

On 2011/04/21 14:37:18, commit-bot wrote:
> Change committed as 82484

3rd try (punt mildly intended) is a charm.

[John Knottenbelt, 2011-07-22 11:35:50]

http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:31: const
WebKit::WebCString spec(source.spec());
If I am reading the code correctly, spec will still share the underlying
m_private anyway. 

I think we need to break the string apart:

const WebKit::WebCString spec(m.spec().data(), m.spec().length());

See related https://bugs.webkit.org/show_bug.cgi?id=65022

[levin, 2011-07-22 12:55:09]

On 2011/07/22 11:35:50, John Knottenbelt wrote:
>
http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
> File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):
> 
>
http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
> webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:31: const
> WebKit::WebCString spec(source.spec());
> If I am reading the code correctly, spec will still share the underlying
> m_private anyway. 
> 
> I think we need to break the string apart:
> 
> const WebKit::WebCString spec(m.spec().data(), m.spec().length());
> 
> See related https://bugs.webkit.org/show_bug.cgi?id=65022

Ah there is still an issue here. I needed to disable the assert that I'm trying
to add https://bugs.webkit.org/show_bug.cgi?id=31639

I haven't had time to look because I've been trying very hard to add
https://bugs.webkit.org/show_bug.cgi?id=31639 :)

[Dmitry Lomov (no reviews), 2011-07-22 17:36:14]

Yikes. Thanks for investigating this John!

On 2011/07/22 11:35:50, John Knottenbelt wrote:
>
http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
> File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):
> 
>
http://codereview.chromium.org/6878059/diff/7/webkit/tools/test_shell/test_sh...
> webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:31: const
> WebKit::WebCString spec(source.spec());
> If I am reading the code correctly, spec will still share the underlying
> m_private anyway. 
> 
> I think we need to break the string apart:
> 
> const WebKit::WebCString spec(m.spec().data(), m.spec().length());
> 
> See related https://bugs.webkit.org/show_bug.cgi?id=65022