Bug 136473 – Avoid warning if a process does not have access to com.apple.webinspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Some sandboxed processes will warn that they do not have access to the com.apple.webinspector mach port. Applications will automatically get the JavaScriptCore framework sandbox extensions to access the port but it seems some processes don't. In such cases the sandbox warning in syslog can be avoided because it is just noise.

<rdar://problem/16647344>

[Joseph Pecoraro]

Created attachment 237539 [details]
[PATCH] Proposed Fix

Still don't know how I feel about this but here is the patch.

[WebKit Commit Bot]

Attachment 237539 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:69:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 237539 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:59
> +#if __has_include(<sandbox/private.h>)
> +#import <sandbox/private.h>
> +#else
> +enum sandbox_filter_type {
> +    SANDBOX_FILTER_GLOBAL_NAME,
> +};
> +extern "C" {
> +int sandbox_check(pid_t, const char *operation, enum sandbox_filter_type, ...);
> +}
> +#endif

Sandbox SPIs seem like a prime target for the new technique Maciej is suggesting, as we use them in many places. Instead of declaring the SPIs in each .mm file, they would go into SandboxSPI.h.

In any case, sandbox_check declaration should not be inside the #else.

[Joseph Pecoraro]

(In reply to comment #3)
> (From update of attachment 237539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237539&action=review
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:59
> > +#if __has_include(<sandbox/private.h>)
> > +#import <sandbox/private.h>
> > +#else
> > +enum sandbox_filter_type {
> > +    SANDBOX_FILTER_GLOBAL_NAME,
> > +};
> > +extern "C" {
> > +int sandbox_check(pid_t, const char *operation, enum sandbox_filter_type, ...);
> > +}
> > +#endif
> 
> Sandbox SPIs seem like a prime target for the new technique Maciej is suggesting, as we use them in many places. Instead of declaring the SPIs in each .mm file, they would go into SandboxSPI.h.

I suppose I could. This would be the first in JavaScriptCore so I opted for the easier path.

> In any case, sandbox_check declaration should not be inside the #else.

I don't follow. Why not?

[Alexey Proskuryakov]

> > In any case, sandbox_check declaration should not be inside the #else.
> 
> I don't follow. Why not?

This way, we'll get a compile error internally if the function signature changes in the future. That's much better than having to chase a mysterious behavior difference between internal and open source builds.

[Joseph Pecoraro]

(In reply to comment #5)
> > > In any case, sandbox_check declaration should not be inside the #else.
> > 
> > I don't follow. Why not?
> 
> This way, we'll get a compile error internally if the function signature changes in the future. That's much better than having to chase a mysterious behavior difference between internal and open source builds.

Internally we should just get the include, so if the signature changes we would fail immediately. If the file changes, then we'd be out of luck.

    #if __has_include(<sandbox/private.h>)
    #import <sandbox/private.h>
    ...

I think we have to put some signature in the else, so that open source developers can compile the file. This pattern is repeated in a couple places (WebKit2).

[Alexey Proskuryakov]

> if the signature changes we would fail immediately

This depends on how the signature changes - it can be source compatible, but not binary compatible. Having an unconditional declaration after the conditional include would catch that.

[Joseph Pecoraro]

Created attachment 237587 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 237587 [details]
[PATCH] For Landing

Clearing flags on attachment: 237587

Committed r173238: <http://trac.webkit.org/changeset/173238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.