| Summary: | [WPE][GTK] GVariant decoding must copy the serialized data | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
| Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bugs-noreply, cgarcia, lmoura, mcatanzaro | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Catanzaro
2020-05-27 15:40:01 PDT
Created attachment 400395 [details]
Patch
Comment on attachment 400395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400395&action=review > Source/WebKit/ChangeLog:15 > + Bonus problem: the GVariant itself is leaked because we are missing adoptGRef. This is not true, g_variant_new returns a floating reference that shouldn't be adopted. > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:65 > + return adoptGRef(g_variant_new_from_bytes(variantType.get(), bytes.get(), FALSE)); Remove the adopt and fix the changelog before landing, please. (In reply to Carlos Garcia Campos from comment #3) > This is not true, g_variant_new returns a floating reference that shouldn't > be adopted. Ugh, you're right! Floating refs plus GRefPtr is a trap. :/ Maybe it would make sense for adoptGRef() to do if (g_object_is_floating()) g_object_ref_sink(), so that it would be safe to use always and would only sink floating refs and otherwise not affect the refcount... dunno, something to think about. Created attachment 400456 [details]
Patch for landing
Committed r262242: <https://trac.webkit.org/changeset/262242> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400456 [details]. Both GTK and WPE build bots are broken after this patch (but due to some other issue, they still report green, I'm investigating this).
Sample error from WPE (same error in other bots):
In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-50d0d8dd-13.cpp:7:
../../Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp: In function ‘WTF::Optional<WTF::GRefPtr<_GVariant> > IPC::decode(IPC::Decoder&)’:
../../Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:65:36: error: could not convert ‘g_variant_new_from_bytes(((const GVariantType*)variantType.std::unique_ptr<_GVariantType, WTF::GPtrDeleter<_GVariantType> >::get()), bytes.WTF::GRefPtr<_GBytes>::get(), 0)’ from ‘GVariant*’ {aka ‘_GVariant*’} to ‘WTF::Optional<WTF::GRefPtr<_GVariant> >’
65 | return g_variant_new_from_bytes(variantType.get(), bytes.get(), FALSE);
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| GVariant* {aka _GVariant*}
(In reply to Michael Catanzaro from comment #4) > (In reply to Carlos Garcia Campos from comment #3) > > This is not true, g_variant_new returns a floating reference that shouldn't > > be adopted. > > Ugh, you're right! Floating refs plus GRefPtr is a trap. :/ > > Maybe it would make sense for adoptGRef() to do if (g_object_is_floating()) > g_object_ref_sink(), so that it would be safe to use always and would only > sink floating refs and otherwise not affect the refcount... dunno, something > to think about. If my memory serves me well, we discussed this as a possibility in one of the Web engines hackfests and the conclusion was that it's a potentially breaking change that would need manual inspection of all the callsites where pointers are adopted to make sure the right thing is done… so it seemed that leaving things as-is was better, and people should take care of knowing when creating an instance results in a floating ref or not 🤔️ |