Bug 190092 - CRASH in CVPixelBufferGetBytePointerCallback()
Summary: CRASH in CVPixelBufferGetBytePointerCallback()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-28 14:32 PDT by Jer Noble
Modified: 2018-10-03 12:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.61 KB, patch)
2018-09-28 14:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.58 KB, patch)
2018-09-28 14:53 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.83 KB, patch)
2018-09-28 16:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2018-10-01 10:40 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-09-28 14:32:29 PDT
CRASH in CVPixelBufferGetBytePointerCallback()
Comment 1 Jer Noble 2018-09-28 14:32:57 PDT
<rdar://43824620>
Comment 2 Radar WebKit Bug Importer 2018-09-28 14:33:33 PDT
<rdar://problem/44875084>
Comment 3 Jer Noble 2018-09-28 14:36:11 PDT
Created attachment 351114 [details]
Patch
Comment 4 Eric Carlson 2018-09-28 14:41:00 PDT
Comment on attachment 351114 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:108
> +    --info->lockCount;

Might be worth adding something like "ASSERT(info->lockCount > 0)"
Comment 5 Tim Horton 2018-09-28 14:47:10 PDT
Comment on attachment 351114 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:128
> +            CVReturn result = CVPixelBufferLockBaseAddress(info->pixelBuffer.get(), kCVPixelBufferLock_ReadOnly);

I think you want an unlock here.
Comment 6 Jer Noble 2018-09-28 14:53:23 PDT
Created attachment 351115 [details]
Patch

Removed the Lock from the info struct, fixed the incorrect lock in the release info callback, and added release logging of the CVPixelBuffer size in the get byte pointer callback.
Comment 7 Jer Noble 2018-09-28 14:54:07 PDT
<rdar://problem/43824620>
Comment 8 Jer Noble 2018-09-28 15:06:58 PDT
Comment on attachment 351114 [details]
Patch

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

>> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:108
>> +    --info->lockCount;
> 
> Might be worth adding something like "ASSERT(info->lockCount > 0)"

Good idea.
Comment 9 Jer Noble 2018-09-28 16:35:17 PDT
Created attachment 351138 [details]
Patch
Comment 10 Said Abou-Hallawa 2018-10-01 10:18:04 PDT
Comment on attachment 351138 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:61
> +    int lockCount { 0 };

Since you changed the pointer type to be RetainPtr<>, can't we get rid of lockCount and use

CFRetain(pixelBuffer.get())
CFRelease(pixelBuffer.get())
CFGetRetainCount(pixelBuffer.get())

Instead of:
++info->lockCount
--info->lockCount
info->lockCount

I think it is conceptually correct as well. You can consider the lock count as a ref/retain count.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:83
> +    ++info->lockCount;

Is this code thread-safe?

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:85
> +    RELEASE_LOG_INFO(Media, "CVPixelBufferGetBytePointerCallback() returning bytePointer: %p, size: %zu", address, CVPixelBufferGetDataSize(info->pixelBuffer.get()));

How often does this function get called? If it's called many times, I would suggest removing this particular RELEASE_LOG_INFO statement because it might add tons of unneeded information.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:139
> +    info->pixelBuffer = nullptr;
> +    delete info;

If you change info to std::unique_ptr or you pass RetainPtr<CVPixelBufferRef>, you will not have to do any of these releases. The pointer will be delete itself once the caller of CVPixelBufferReleaseInfoCallback() releases the last reference to this pointer.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:180
> +    CVPixelBufferInfo* info = new CVPixelBufferInfo();

We usually do not do bare "new" calls. 

Can't we do it this way?
    CVPixelBufferInfo* info = std::make_unique<CVPixelBufferInfo>().release();

Or can't we make info a std::unique_ptr?
    auto info = std::make_unique<CVPixelBufferInfo>();

Or can't we pass the RetainPtr itself instead of the "info" pointer and use CFRetain/CFRelease instead of lockCount?
    RetainPtr<CVPixelBufferRef> pixelBuffer = WTFMove(buffer);
Comment 11 Jer Noble 2018-10-01 10:28:53 PDT
(In reply to Said Abou-Hallawa from comment #10)
> Comment on attachment 351138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351138&action=review
> 
> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:61
> > +    int lockCount { 0 };
> 
> Since you changed the pointer type to be RetainPtr<>, can't we get rid of
> lockCount and use
> 
> CFRetain(pixelBuffer.get())
> CFRelease(pixelBuffer.get())
> CFGetRetainCount(pixelBuffer.get())
> 
> Instead of:
> ++info->lockCount
> --info->lockCount
> info->lockCount

No, because that would turn a logging change into a behavior change. We don't want to map locking the pixel buffer onto ref'ing the buffer itself, especially if the client isn't matching up its locks and unlocks.

> I think it is conceptually correct as well. You can consider the lock count
> as a ref/retain count.

No, you absolutely cannot. The lock count is on the CVPixelBuffer's internal storage, not the CVPixelBuffer itself.  Their lifetimes are not the same, and we can't treat them as the same.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:83
> > +    ++info->lockCount;
> 
> Is this code thread-safe?

Nope!  But again, this is just a logging change.  If the caller is calling lock and unlock simultaneously on different threads, there's already huge problems because the pointer returned by lock() won't be valid after unlock() is called.+

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:85
> > +    RELEASE_LOG_INFO(Media, "CVPixelBufferGetBytePointerCallback() returning bytePointer: %p, size: %zu", address, CVPixelBufferGetDataSize(info->pixelBuffer.get()));
> 
> How often does this function get called? If it's called many times, I would
> suggest removing this particular RELEASE_LOG_INFO statement because it might
> add tons of unneeded information.

According to my testing, this is called twice per enter- or exit-fullscreen operation, which is not very often. The entire purpose of this logging is to allow us to co-relate the crash location to what CV thinks the size of the buffer is, so I think this is particularly essential logging.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:139
> > +    info->pixelBuffer = nullptr;
> > +    delete info;
> 
> If you change info to std::unique_ptr or you pass
> RetainPtr<CVPixelBufferRef>, you will not have to do any of these releases.
> The pointer will be delete itself once the caller of
> CVPixelBufferReleaseInfoCallback() releases the last reference to this
> pointer.

Not sure I really understand your point here. The CVPixelBuffer is stored in a RetainPtr.  And info can't be stored in a std::unique_ptr, since it's just passed around as a void*.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:180
> > +    CVPixelBufferInfo* info = new CVPixelBufferInfo();
> 
> We usually do not do bare "new" calls. 
> 
> Can't we do it this way?
>     CVPixelBufferInfo* info =
> std::make_unique<CVPixelBufferInfo>().release();

How does that solve anything? That's just a complicated wrapper around 'new'.

> Or can't we make info a std::unique_ptr?
>     auto info = std::make_unique<CVPixelBufferInfo>();
> 
> Or can't we pass the RetainPtr itself instead of the "info" pointer and use
> CFRetain/CFRelease instead of lockCount?
>     RetainPtr<CVPixelBufferRef> pixelBuffer = WTFMove(buffer);

No.

Using a NSObject* instead of a C++ object is just kicking the can down the road. It doesn't solve anything on its own. This data structure still needs to be passed around as a void* and cast to a type.
Comment 12 Jer Noble 2018-10-01 10:40:23 PDT
Created attachment 351263 [details]
Patch

One more fix for the bots.
Comment 13 WebKit Commit Bot 2018-10-03 12:28:39 PDT
Comment on attachment 351263 [details]
Patch

Clearing flags on attachment: 351263

Committed r236806: <https://trac.webkit.org/changeset/236806>
Comment 14 WebKit Commit Bot 2018-10-03 12:28:41 PDT
All reviewed patches have been landed.  Closing bug.