Bug 174407 – H264VideoToolboxDecoder::Decode has a memory leak when homing out on iOS

• Component: Media
• Status: RESOLVED CONFIGURATION CHANGED

[youenn fablet]

H264VideoToolboxDecoder::Decode has a memory leak when homing out on iOS

[youenn fablet]

rdar://problem/33246646

[youenn fablet]

Created attachment 315197 [details]
Patch

[Build Bot]

Attachment 315197 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:146:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 315197 [details]
Patch

Does VTDecompressionSessionDecodeFrame delete the pointer when it's done?  I think this whole function's memory management needs to be made much more clear.  I think the first new should be replaced with std::make_unique.  Do we need to manually delete the released unique_ptr if both calls fail?

[Build Bot]

Comment on attachment 315197 [details]
Patch

Attachment 315197 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4105460

New failing tests:
webrtc/captureCanvas-webrtc.html

[Build Bot]

Created attachment 315213 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[youenn fablet]

> Does VTDecompressionSessionDecodeFrame delete the pointer when it's done?  I
> think this whole function's memory management needs to be made much more
> clear.  I think the first new should be replaced with std::make_unique.  Do
> we need to manually delete the released unique_ptr if both calls fail?

VTDecompressionSessionDecodeFrame is just getting a pointer and passing it to the callback that retakes it as a unique_ptr.
As long as VTDecompressionSessionDecodeFrame does not call the callback when returning an error result, this patch is fine.

I'll change the patch to use make_unique instead of the current code.

[youenn fablet]

(In reply to Build Bot from comment #5)
> Comment on attachment 315197 [details]
> Patch
> 
> Attachment 315197 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/4105460
> 
> New failing tests:
> webrtc/captureCanvas-webrtc.html

Hum, this test is crashing probably because of my patch, I'll check that further.

[youenn fablet]

Created attachment 315310 [details]
Patch

[youenn fablet]

> As long as VTDecompressionSessionDecodeFrame does not call the callback when
> returning an error result, this patch is fine.

This assumption does not hold, I have changed the patch to just remove the unnecessary double allocation. This should fix the leak in the case I spotted.
This does not fix all potential cases I guess.
A future patch should do proper handling there.

[Build Bot]

Attachment 315310 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:141:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:144:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:156:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 315310 [details]
Patch

Both callbacks are responsible for deleting the FrameDecodeParams* with this design.  This is *very* bad.  We need to refcount instead if we give two callbacks ownership of the object.