Bug 115448 – ipc-posix-shm backwards compatibility

• Component: WebKit2
• Status: RESOLVED FIXED

[Simon Cooper]

Need backwards compatibility for older ipc-posix-shm operations.

[Simon Cooper]

Created attachment 200194 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 200194 [details]
Patch

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

r=me

> Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.sb:250
> +(if (not (defined? 'ipc-posix-shm*))
> +    (define ipc-posix-shm* ipc-posix-shm))

I wish we pre-processed plug-in sandbox profile files, so that we could do conditional compilation. With an #if check, removing obsolete code is much more straightforward.

Also, we wouldn't have to merge parts at runtime, as we could simply #include "com.apple.WebKit.plugin-common.sb", and let preprocessor build a complete profile.

[WebKit Commit Bot]

Comment on attachment 200194 [details]
Patch

Clearing flags on attachment: 200194

Committed r149427: <http://trac.webkit.org/changeset/149427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Cooper]

(In reply to comment #2)
> (From update of attachment 200194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200194&action=review
> 
> r=me
> 
> > Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.sb:250
> > +(if (not (defined? 'ipc-posix-shm*))
> > +    (define ipc-posix-shm* ipc-posix-shm))
> 
> I wish we pre-processed plug-in sandbox profile files, so that we could do conditional compilation. With an #if check, removing obsolete code is much more straightforward.

This isn't obsolete code -- it provides backwards compatibility.

> Also, we wouldn't have to merge parts at runtime, as we could simply #include "com.apple.WebKit.plugin-common.sb", and let preprocessor build a complete profile.

This is not a good idea. 

- This will not work if the AppleConnect profile is moved to a different project.

- Forcing cpp preprocessing means that some sort of  "compilation step" has to occur before passing to sandbox which makes it harder to develop and debug profiles.

[Alexey Proskuryakov]

> This isn't obsolete code -- it provides backwards compatibility.

It will be obsolete relatively soon, at which point we'll certainly forget to remove it, because there is no indication in code about when it can be removed. We know this currently, and being good forward looking engineers, we can pave the path now.

Conditional compilation macros is how we do this in other code, so this would be the most straightforward way for profiles too.

> - This will not work if the AppleConnect profile is moved to a different project.

I can't agree with that. In fact, it's the opposite - having half of the profile in WebKit and another half in a different project is not workable long term.

> - Forcing cpp preprocessing means that some sort of  "compilation step" has to occur before passing to sandbox which makes it harder to develop and debug profiles.

We perform preprocessing for some of the profiles, and haven't had any issues with that yet.

[Simon Cooper]

(In reply to comment #6)
> > This isn't obsolete code -- it provides backwards compatibility.
> 
> It will be obsolete relatively soon, at which point we'll certainly forget to remove it, because there is no indication in code about when it can be removed. We know this currently, and being good forward looking engineers, we can pave the path now.

If that is the issue, it can certainly be addressed.  What specifically do you need to see?

[Alexey Proskuryakov]

CC'ing Mark, as I think that he is the one who most frequently does such cleanup. I _think_ that preprocessor checks like "#if __MAC_OS_X_VERSION_MIN_REQUIRED == 1070" would make it easiest.

Alternatively, a FIXME comment explaining when the code can be removed would help us do that eventually, even if with a bit more manual work.