Bug 221224 – [WPE][GTK] BubblewrapLauncher should create flatpak-info keyfile only once

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

BubblewrapLauncher should create its flatpak-info keyfile only once, because its contents will never change. Makes more sense to cache this tiny string in memory than to recompute it every time a subprocess is launched.

[Michael Catanzaro]

Created attachment 418918 [details]
Patch

[Adrian Perez]

Comment on attachment 418918 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:697
> +    static GUniquePtr<char> data;

Shouldn't this be better be NeverDestroyed<CString> instead?
Or even MainThreadNeverDestroyed<CString> ;-)

[Michael Catanzaro]

NeverDestroyed is required for static objects in cross-platform code to ensure we don't run destructors, because the order the destructors execute is undefined on Windows. Historically, I don't think I've been using it in our platform-specific code since this problem just doesn't exist on Linux. I guess that's maybe not a great idea, though, since it could cause trouble if the code is eventually ever ported to Windows. (On the other hand, we can be fairly confident BubblewrapLauncher.cpp cannot possibly ever work on Windows, so shrug.)

Anyway, I'll try changing it to NeverDestroyed<CString>.

MainThreadNeverDestroyed is only needed if we have to assert that the object is never accessed on another thread, so normal NeverDestroyed is fine here.

[Michael Catanzaro]

Comment on attachment 418918 [details]
Patch

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

>> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:697
>> +    static GUniquePtr<char> data;
> 
> Shouldn't this be better be NeverDestroyed<CString> instead?
> Or even MainThreadNeverDestroyed<CString> ;-)

Hm, CString doesn't work well here. I need the GUniquePtr<char> to hold ownership of the result of g_key_file_to_data(). Creating a CString from it would require an extra copy, and a second variable, and wouldn't accomplish anything.

So the good options here are either:

static NeverDestroyed<char*> data;

Or:

static GUniquePtr<char> data; (what I have already)

Or, perhaps overkill:

static NeverDestroyed<GUniquePtr<char>> data;

[Michael Catanzaro]

Ping Adrian. I have a merge conflict now between this and bug #219010.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 418918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418918&action=review
> 
> >> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:697
> >> +    static GUniquePtr<char> data;
> > 
> > Shouldn't this be better be NeverDestroyed<CString> instead?
> > Or even MainThreadNeverDestroyed<CString> ;-)
> 
> Hm, CString doesn't work well here. I need the GUniquePtr<char> to hold
> ownership of the result of g_key_file_to_data(). Creating a CString from it
> would require an extra copy, and a second variable, and wouldn't accomplish
> anything.
> 
> So the good options here are either:
> 
> static NeverDestroyed<char*> data;
> 
> Or:
> 
> static GUniquePtr<char> data; (what I have already)
> 
> Or, perhaps overkill:
> 
> static NeverDestroyed<GUniquePtr<char>> data;

I would go with this option, there are similar uses around the codebase
e.g. NeverDestroyed<GRefPtr<…>> and NeverDestroyed<Ref<…> :)

[Michael Catanzaro]

Created attachment 420064 [details]
Patch

[Michael Catanzaro]

Ping Adrian :)

[EWS]

Committed r272854: <https://commits.webkit.org/r272854>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420064 [details].