[webkit-reviews] review denied: [Bug 36723] Crash while uploading a PDF document on www.largefilesasap.com : [Attachment 52120] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 36723: Crash while uploading a PDF document on www.largefilesasap.com
https://bugs.webkit.org/show_bug.cgi?id=36723

Attachment 52120: Patch
https://bugs.webkit.org/attachment.cgi?id=52120&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
Thanks for making this change.

> @@ -79,13 +81,13 @@ void FileChooser::chooseFiles(const Vector<String>&
filenames)
>  void FileChooser::loadIcon()
>  {
>      if (m_filenames.size() && m_client)
> -	   m_client->chooseIconForFiles(m_filenames);
> +	   m_client->chooseIconForFiles(this, m_filenames);
>  }
> diff --git a/WebCore/rendering/RenderFileUploadControl.cpp
b/WebCore/rendering/RenderFileUploadControl.cpp
> index 14d126d..a66b118 100644
> --- a/WebCore/rendering/RenderFileUploadControl.cpp
> +++ b/WebCore/rendering/RenderFileUploadControl.cpp
> @@ -114,10 +114,10 @@ String RenderFileUploadControl::acceptTypes()
> -void RenderFileUploadControl::chooseIconForFiles(const Vector<String>&
filenames)
> +void RenderFileUploadControl::chooseIconForFiles(PassRefPtr<FileChooser>
chooser, const Vector<String>& filenames)
>  {
>      if (Chrome* chromePointer = chrome())
> -	   chromePointer->chooseIconForFiles(filenames, m_fileChooser);
> +	   chromePointer->chooseIconForFiles(filenames, chooser);
>  }

> diff --git a/WebCore/rendering/RenderFileUploadControl.h
b/WebCore/rendering/RenderFileUploadControl.h
> index 99dd35c..9714db1 100644
> --- a/WebCore/rendering/RenderFileUploadControl.h
> +++ b/WebCore/rendering/RenderFileUploadControl.h
> @@ -61,7 +61,7 @@ private:
> -    void chooseIconForFiles(const Vector<String>&);
> +    void chooseIconForFiles(PassRefPtr<FileChooser>, const Vector<String>&);


PassRefPtr is wrong here.

In the original bug, Darin pointed this out
(https://bugs.webkit.org/show_bug.cgi?id=35072#c8) and you described why you
thought this qualified (https://bugs.webkit.org/show_bug.cgi?id=35072#c12), and
I think someone should've followed up on this.

PassRefPtr is actually about *transferring* ownership.	It's meant to be used
when you're transferring from one RefPtr to another RefPtr.  In that case, it
zeroes out the old RefPtr without decrementing the ref count.  It then assigns
the raw pointer to the new RefPtr without incrementing the ref count.  This
allows ownership transfer without (surprisingly expensive) ref count churn.

Creating this PassRefPtr from the raw pointer *always* bumps the ref count,
even if the client isn't interested in hanging on to the object.  Then the
~PassRefPtr decrements the ref count, even when the client never cared.  That's
the churn that PassRefPtr is actually designed to avoid.

This case wasn't ever actually *transferring* ownership.  While it's true that
Chromium needs to ref the object to hang on to it, the RenderFileUploadControl
doesn't actually *give up* its ref.  In this regard, the PassRefPtr is wrong in
all of the client methods originally implemented in each of the WebKits.

This really can be a raw pointer.  In the common platform (all but Chromium)
that don't need a ref and that synchronously call back into WebCore, not
ref'ing is correct.  In the Chromium case, it has to bump the ref count
anyways.

I'm tempted to ask you to update all of the client methods to be raw pointers,
but since this isn't really a hot code path, I won't insist.

However, I will insist that this new method be a raw pointer, so we don't carry
the mistake all the way to the core.  :)