Bug 146995 - Reduce PassRefPtr in WebKit2 - 3
Summary: Reduce PassRefPtr in WebKit2 - 3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 144092
  Show dependency treegraph
 
Reported: 2015-07-15 22:56 PDT by Gyuyoung Kim
Modified: 2015-07-19 06:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (98.76 KB, patch)
2015-07-15 23:04 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (115.05 KB, patch)
2015-07-16 07:23 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (115.13 KB, patch)
2015-07-16 17:51 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (116.17 KB, patch)
2015-07-16 18:51 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (116.22 KB, patch)
2015-07-18 01:49 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (116.22 KB, patch)
2015-07-18 04:17 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (116.23 KB, patch)
2015-07-18 06:06 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (116.19 KB, patch)
2015-07-18 15:33 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>