Bug 146995

Summary:

Reduce PassRefPtr in WebKit2 - 3

Product:

WebKit

Reporter:

Gyuyoung Kim <gyuyoung.kim>

Component:

New Bugs

Assignee:

Gyuyoung Kim <gyuyoung.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, commit-queue, darin, dbates, ddkilzer, kling, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

144092

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Description Gyuyoung Kim 2015-07-15 22:56:51 PDT

SSIA

Comment 1 Gyuyoung Kim 2015-07-15 23:04:20 PDT

Created attachment 256893 [details]
Patch

Comment 2 Gyuyoung Kim 2015-07-16 07:23:41 PDT

Created attachment 256900 [details]
Patch

Comment 3 Gyuyoung Kim 2015-07-16 17:51:05 PDT

Created attachment 256940 [details]
Patch

Comment 4 Gyuyoung Kim 2015-07-16 18:51:39 PDT

Created attachment 256944 [details]
Patch

Comment 5 Andreas Kling 2015-07-17 13:36:03 PDT

Comment on attachment 256944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256944&action=review

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333
> -    return loader.release();
> +    return loader.leakRef();

Doesn't this introduce a leak?
Same question for every other instance of release() -> leakRef() in this patch.

Comment 6 Daniel Bates 2015-07-17 22:22:01 PDT

Comment on attachment 256944 [details]
Patch

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333
> -    return loader.release();
> +    return loader.leakRef();

This is not the correct idiom. It should be sufficient to write this line as:

return loader;

Comment 7 Daniel Bates 2015-07-17 23:02:24 PDT

(In reply to comment #6)
> Comment on attachment 256944 [details]
> Patch
> 
> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333
> > -    return loader.release();
> > +    return loader.leakRef();
> 
> This is not the correct idiom. It should be sufficient to write this line as:
> 
> return loader;

err, I meant to write:

return WTF::move(loader);

Comment 8 Gyuyoung Kim 2015-07-18 01:49:22 PDT

Created attachment 257026 [details]
Patch

Comment 9 Gyuyoung Kim 2015-07-18 01:50:05 PDT

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 256944 [details]
> > Patch
> > 
> > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333
> > > -    return loader.release();
> > > +    return loader.leakRef();
> > 
> > This is not the correct idiom. It should be sufficient to write this line as:
> > 
> > return loader;
> 
> err, I meant to write:
> 
> return WTF::move(loader);

Oops, that was my fault. Thank you for pointing it out.

Comment 10 Gyuyoung Kim 2015-07-18 04:17:36 PDT

Created attachment 257028 [details]
Patch

Comment 11 Gyuyoung Kim 2015-07-18 06:06:50 PDT

Created attachment 257029 [details]
Patch

Comment 12 Daniel Bates 2015-07-18 08:58:50 PDT

Comment on attachment 257029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257029&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:251
> +    return WTF::move(page);

It is unnecessary to use WTF::move() here. I would write this line as:

return page;

Comment 13 Gyuyoung Kim 2015-07-18 15:33:55 PDT

Created attachment 257033 [details]
Patch for landing

Comment 14 WebKit Commit Bot 2015-07-18 17:18:07 PDT

Comment on attachment 257033 [details]
Patch for landing

Clearing flags on attachment: 257033

Committed r187002: <http://trac.webkit.org/changeset/187002>

Comment 15 WebKit Commit Bot 2015-07-18 17:18:14 PDT

All reviewed patches have been landed.  Closing bug.

Comment 16 David Kilzer (:ddkilzer) 2015-07-19 05:42:55 PDT

This broken the Windows build:

<https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/71645/steps/compile-webkit/logs/stdio>

..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(954): error C2440: 'initializing' : cannot convert from 'WTF::PassRefPtr<T>' to 'WTF::Ref<T>' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          with
          [
              T=WebDocumentLoader
          ]
          No constructor could take the source type, or constructor overload resolution was ambiguous
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(956): error C2664: 'WebDataSource *WebDataSource::createInstance(WebDocumentLoader *)' : cannot convert argument 1 from 'WebDocumentLoader' to 'WebDocumentLoader *' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(956): error C2664: 'COMPtr<WebDataSource>::COMPtr(WTF::HashTableDeletedValueType)' : cannot convert argument 1 from 'AdoptCOMTag' to 'WebDataSource *' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1051): error C2556: 'WTF::PassRefPtr<WebCore::Frame> WebFrameLoaderClient::createFrame(const WebCore::URL &,const WTF::String &,WebCore::HTMLFrameOwnerElement *,const WTF::String &,bool,int,int)' : overloaded function differs only by return type from 'WTF::RefPtr<WebCore::Frame> WebFrameLoaderClient::createFrame(const WebCore::URL &,const WTF::String &,WebCore::HTMLFrameOwnerElement *,const WTF::String &,bool,int,int)' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(183) : see declaration of 'WebFrameLoaderClient::createFrame'
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1051): error C2371: 'WebFrameLoaderClient::createFrame' : redefinition; different basic types [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(183) : see declaration of 'WebFrameLoaderClient::createFrame'
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1168): error C2556: 'WTF::PassRefPtr<WebCore::Widget> WebFrameLoaderClient::createPlugin(const WebCore::IntSize &,WebCore::HTMLPlugInElement *,const WebCore::URL &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::String &,bool)' : overloaded function differs only by return type from 'WTF::RefPtr<WebCore::Widget> WebFrameLoaderClient::createPlugin(const WebCore::IntSize &,WebCore::HTMLPlugInElement *,const WebCore::URL &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::String &,bool)' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(185) : see declaration of 'WebFrameLoaderClient::createPlugin'
..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1168): error C2371: 'WebFrameLoaderClient::createPlugin' : redefinition; different basic types [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(185) : see declaration of 'WebFrameLoaderClient::createPlugin'

Comment 17 David Kilzer (:ddkilzer) 2015-07-19 06:18:47 PDT

Attempted build fix for Windows in r187010:  <http://trac.webkit.org/r187010>