168396 2017-02-15 15:15:10 -0800 ImageFrame has to implement its copy constructor 2017-07-24 10:54:17 -0700 1 1 1 Unclassified WebKit Images WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 172552 InRadar P2 Normal --- 1 sabouhallawa sabouhallawa simon.fraser thorton oldest_to_newest 1277379 0 sabouhallawa 2017-02-15 15:15:10 -0800 Currently the ImageFrame copy constructor calls the assignment operator. This is a bad idea because the members of the object aren't initialized properly when calling the assignment operator. The problematic member is m_nativeImage. Assigning a new RetainPtr to m_nativeImage will force calling the destructor of the old RetainPtr which is garbage at that time. This may lead to the following crash: Thread[0] EXC_BAD_ACCESS (SIGBUS) (KERN_PROTECTION_FAILURE at 0x00000001123fd000) [ 0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WTF::RetainPtr<CGImage*>::RetainPtr() at RetainPtr.h:64:19 [ 0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WTF::RetainPtr<CGImage*>::RetainPtr() at RetainPtr.h:64 [ 0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WebCore::ImageFrame::ImageFrame() + 20 at ImageFrame.cpp:33 [ 1] 0x0000000107894a27 WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::VectorInitializer<true, false, WebCore::ImageFrame>::initialize(WebCore::ImageFrame*, WebCore::ImageFrame*) + 28 at Vector.h:79:32 [ 1] 0x0000000107894a0b WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::VectorTypeOperations<WebCore::ImageFrame>::initialize(WebCore::ImageFrame*, WebCore::ImageFrame*) at Vector.h:229 [ 1] 0x0000000107894a0b WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::Vector<WebCore::ImageFrame, 1ul, WTF::CrashOnOverflow, 16ul>::grow(unsigned long) + 77 at Vector.h:1036 [ 1] 0x00000001078949be WebCore`WebCore::ImageFrameCache::growFrames() + 46 at ImageFrameCache.cpp:165 [ 2] 0x0000000107ca9a1a WebCore`WebCore::ImageSource::dataChanged(WebCore::SharedBuffer*, bool) + 90 at ImageSource.cpp:155:5 1277380 1 301662 sabouhallawa 2017-02-15 15:21:43 -0800 Created attachment 301662 Patch 1277381 2 sabouhallawa 2017-02-15 15:23:16 -0800 <rdar://problem/28905851> 1277386 3 301662 simon.fraser 2017-02-15 15:26:47 -0800 Comment on attachment 301662 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review > Source/WebCore/ChangeLog:8 > + Calling the assignment operator form its copy constructor is a bad idea. form -> from > Source/WebCore/ChangeLog:11 > + assignment operator. Assigning a new RetainPtr to m_nativeImage will force > + calling the destructor of the old RetainPtr which is garbage at that time. I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from? 1277397 4 301662 simon.fraser 2017-02-15 15:42:05 -0800 Comment on attachment 301662 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review >> Source/WebCore/ChangeLog:11 >> + calling the destructor of the old RetainPtr which is garbage at that time. > > I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from? I now understand, but you should say it more clearly, that in a copy constructor, the data members have not yet been initialized. > Source/WebCore/platform/graphics/ImageFrame.cpp:112 > + *this = defaultFrame(); This is a bit of a weird way to reset the data members. Usually we just reset them manually. 1277442 5 301662 sabouhallawa 2017-02-15 17:57:06 -0800 Comment on attachment 301662 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review >>> Source/WebCore/ChangeLog:11 >>> + calling the destructor of the old RetainPtr which is garbage at that time. >> >> I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from? > > I now understand, but you should say it more clearly, that in a copy constructor, the data members have not yet been initialized. I think I am wrong here and I confused you. I realized that In general calling the assignment operator from the copy constructor is a bad idea. The reason is the members of the class *may* not be properly initialized at that time. -- For example this code has a bug: class A { public: A() : m_ptr(nullptr) { } A(const A& other) { *this = other; } A& operator=(const A& other) { if (m_ptr) delete [] m_ptr; // BUG: m_ptr is not initialized when this is called form the copy constructor .... } private: char* m_ptr; }; -- This code is fine but it is ugly: class B { public: B() { } B(const B& other) { *this = other; } B& operator=(const A& other) { if (m_ptr) delete [] m_ptr; // OK: m_ptr is initialized when this is called form the copy constructor .... } private: char* m_ptr { nullptr }; String m_str; }; The members of the class have to be initialized before executing the body of the constructor. So m_ptr will be set to nullptr then the default constructor of the String object will be called and finally the body of the copy constructor of class B will be executed. Please see the "Initialization order" section in http://en.cppreference.com/w/cpp/language/initializer_list. The ImageFrame is similar to class B above. Its members either have initializers in the class definition or they are classes. This means, the default constructor of the RetainPtr has to be called before entering the body of the copy constructor regardless how it is implemented. So I think making the the copy constructor of ImageFrame calling its assignment operator is ugly but can't cause the crash I mentioned above. 1277443 6 301662 sabouhallawa 2017-02-15 17:58:27 -0800 Comment on attachment 301662 Patch Removing the r+ flag till I figure out the reason of the crash and decide if cleaning the ImageFrame copy constructor is still needed or not. 1331669 7 sabouhallawa 2017-07-24 10:54:17 -0700 The crash below was fixed by <http://trac.webkit.org/changeset/217392>. So this bug can be considered a duplicate of https://bugs.webkit.org/show_bug.cgi?id=172552. *** This bug has been marked as a duplicate of bug 172552 *** 301662 2017-02-15 15:21:43 -0800 2017-02-15 17:58:27 -0800 Patch bug-168396-20170215151907.patch text/plain 2650 sabouhallawa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxMjQwMCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE3LTAyLTE1ICBTYWlkIEFi b3UtSGFsbGF3YSAgPHNhYm91aGFsbGF3YUBhcHBsZS5jb20+CisKKyAgICAgICAgSW1hZ2VGcmFt ZSBoYXMgdG8gaW1wbGVtZW50IGl0cyBjb3B5IGNvbnN0cnVjdG9yCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjgzOTYKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDYWxsaW5nIHRoZSBhc3NpZ25tZW50IG9w ZXJhdG9yIGZvcm0gaXRzIGNvcHkgY29uc3RydWN0b3IgaXMgYSBiYWQgaWRlYS4KKyAgICAgICAg VGhlIG1lbWJlcnMgb2YgdGhlIG9iamVjdCBhcmVuJ3QgaW5pdGlhbGl6ZWQgcHJvcGVybHkgd2hl biBjYWxsaW5nIHRoZQorICAgICAgICBhc3NpZ25tZW50IG9wZXJhdG9yLiBBc3NpZ25pbmcgYSBu ZXcgUmV0YWluUHRyIHRvIG1fbmF0aXZlSW1hZ2Ugd2lsbCBmb3JjZQorICAgICAgICBjYWxsaW5n IHRoZSBkZXN0cnVjdG9yIG9mIHRoZSBvbGQgUmV0YWluUHRyIHdoaWNoIGlzIGdhcmJhZ2UgYXQg dGhhdCB0aW1lLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvSW1hZ2VGcmFtZS5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpJbWFnZUZyYW1lOjpJbWFnZUZyYW1lKToKKyAgICAgICAgKFdl YkNvcmU6OkltYWdlRnJhbWU6OmNsZWFyKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9J bWFnZUZyYW1lLmg6CisgICAgICAgIChXZWJDb3JlOjpJbWFnZUZyYW1lOjpJbWFnZUZyYW1lKTog RGVsZXRlZC4KKwogMjAxNy0wMi0xNSAgSmVyIE5vYmxlICA8amVyLm5vYmxlQGFwcGxlLmNvbT4K IAogICAgICAgICBEaXNhYmxlZCBNZWRpYSBTb3VyY2VzIHNob3VsZCByZW5kZXIgYmxhY2svc2ls ZW5jZQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvSW1hZ2VGcmFtZS5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvSW1hZ2VG cmFtZS5jcHAJKHJldmlzaW9uIDIxMjI2NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL0ltYWdlRnJhbWUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNCw2ICszNCwyNCBAQCBJ bWFnZUZyYW1lOjpJbWFnZUZyYW1lKCkKIHsKIH0KIAorSW1hZ2VGcmFtZTo6SW1hZ2VGcmFtZShj b25zdCBJbWFnZUZyYW1lJiBvdGhlcikKKyAgICA6IG1fZGVjb2Rpbmcob3RoZXIubV9kZWNvZGlu ZykKKyAgICAsIG1fc2l6ZShvdGhlci5tX3NpemUpCisjaWYgIVVTRShDRykKKyAgICAsIG1fZGlz cG9zYWxNZXRob2Qob3RoZXIubV9kaXNwb3NhbE1ldGhvZCkKKyNlbmRpZgorICAgICwgbV9uYXRp dmVJbWFnZShvdGhlci5tX25hdGl2ZUltYWdlKQorICAgICwgbV9zdWJzYW1wbGluZ0xldmVsKG90 aGVyLm1fc3Vic2FtcGxpbmdMZXZlbCkKKyAgICAsIG1fb3JpZW50YXRpb24ob3RoZXIubV9vcmll bnRhdGlvbikKKyAgICAsIG1fZHVyYXRpb24ob3RoZXIubV9kdXJhdGlvbikKKyAgICAsIG1faGFz QWxwaGEob3RoZXIubV9oYXNBbHBoYSkKK3sKKyNpZiAhVVNFKENHKQorICAgIGlmIChvdGhlci5i YWNraW5nU3RvcmUoKSkKKyAgICAgICAgaW5pdGlhbGl6ZSgqb3RoZXIuYmFja2luZ1N0b3JlKCkp OworI2VuZGlmCit9CisKIEltYWdlRnJhbWU6On5JbWFnZUZyYW1lKCkKIHsKICAgICBjbGVhcklt YWdlKCk7CkBAIC05MSw3ICsxMDksNyBAQCB1bnNpZ25lZCBJbWFnZUZyYW1lOjpjbGVhckltYWdl KCkKIHVuc2lnbmVkIEltYWdlRnJhbWU6OmNsZWFyKCkKIHsKICAgICB1bnNpZ25lZCBmcmFtZUJ5 dGVzID0gY2xlYXJJbWFnZSgpOwotICAgICp0aGlzID0gSW1hZ2VGcmFtZSgpOworICAgICp0aGlz ID0gZGVmYXVsdEZyYW1lKCk7CiAgICAgcmV0dXJuIGZyYW1lQnl0ZXM7CiB9CiAKSW5kZXg6IFNv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ltYWdlRnJhbWUuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9JbWFnZUZyYW1lLmgJKHJldmlzaW9u IDIxMjI2NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ltYWdlRnJhbWUu aAkod29ya2luZyBjb3B5KQpAQCAtODUsNyArODUsNyBAQCBwdWJsaWM6CiAgICAgZW51bSBjbGFz cyBEZWNvZGluZyB7IEVtcHR5LCBCZWluZ0RlY29kZWQsIFBhcnRpYWwsIENvbXBsZXRlIH07CiAK ICAgICBJbWFnZUZyYW1lKCk7Ci0gICAgSW1hZ2VGcmFtZShjb25zdCBJbWFnZUZyYW1lJiBvdGhl cikgeyBvcGVyYXRvcj0ob3RoZXIpOyB9CisgICAgSW1hZ2VGcmFtZShjb25zdCBJbWFnZUZyYW1l Jik7CiAKICAgICB+SW1hZ2VGcmFtZSgpOwogCg==