Bug 221224

Created attachment 418918 [details]
Patch

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> ;-)

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.

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;

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

(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<…> :)

Created attachment 420064 [details]
Patch

Ping Adrian :)

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

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