Bug 172352 – NavigatorEME: null RefPtr<> dereference due to different calling conventions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

NavigatorEME: null RefPtr<> dereference due to different calling conventions

[Zan Dobersek]

Created attachment 310643 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 310643 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/NavigatorEME.cpp:92
> +                const Sting& keySystem = implementation->keySystem();

How can this build? Sting -> String Are we really copying it when using const String&

[Build Bot]

Comment on attachment 310643 [details]
Patch

Attachment 310643 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3776143

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-buffered.html

[Build Bot]

Created attachment 310650 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Zan Dobersek]

Comment on attachment 310643 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/NavigatorEME.cpp:92
>> +                const Sting& keySystem = implementation->keySystem();
> 
> How can this build? Sting -> String Are we really copying it when using const String&

Initially it was copying it. Changed it to a reference before uploading, forgot to update the comment, and didn't recompile after the change.

[Zan Dobersek]

Created attachment 310816 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 310816 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/NavigatorEME.cpp:92
> +                // Obtain reference to the key system string before the `implementation` RefPtr<> is cleared out.
> +                const String& keySystem = implementation->keySystem();

I understand now, we are releasing the RefPtr<>, but not implementation that is transferred to the MediaKeySystemAccess, that's why we don't need to copy. Right?

[Zan Dobersek]

Comment on attachment 310816 [details]
Patch

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

>> Source/WebCore/Modules/encryptedmedia/NavigatorEME.cpp:92
>> +                const String& keySystem = implementation->keySystem();
> 
> I understand now, we are releasing the RefPtr<>, but not implementation that is transferred to the MediaKeySystemAccess, that's why we don't need to copy. Right?

Right, the CDM object is still on the heap, it's only RefPtr<> that is invalidated.

[Zan Dobersek]

Comment on attachment 310816 [details]
Patch

Clearing flags on attachment: 310816

Committed r217220: <http://trac.webkit.org/changeset/217220>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.