10.7: Java applets do not work due to sandbox violation/exception

[Simon Cooper]

10.7: Java applets do not work due to sandbox violation/exception

[Simon Cooper]

Created attachment 207147 [details]
Patch

[Simon Cooper]

Created attachment 207161 [details]
Patch

[Simon Cooper]

Created attachment 207162 [details]
Patch

[Simon Cooper]

Created attachment 207216 [details]
Patch

[Simon Cooper]

Created attachment 207224 [details]
Patch

[Simon Cooper]

In my opinion, this version doesn't seem to be an simpler or saner that the purely
Scheme based version.

[Alexey Proskuryakov]

Comment on attachment 207224 [details]
Patch

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

> Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.sb:325
> +(if (equal? os-version "10.7")
> +    (allow ipc-posix-shm)
> +    (begin
> +        (if (equal? os-version "10.8")

This nested code is very hard to read. Preprocessor would make it so much easier, because it has an easy to use wide set of operations, and doesn't force deep nesting:

#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080 
...
#else
...
#endif

Also, preprocessor would have had zero cost at runtime.

> Source/WebKit2/Shared/SandboxInitializationParameters.h:49
> +    void addParameter(const char* name, NSString *value);

You probably did this for consistency with existing addPathParameter function, however it stands out to me as unnecessary to round-trip via an Objective C value in this case.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:173
> +    if(osVersionParts.size() < 2) {

There should be a space between "if" and "(".

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:180
> +    sandboxParameters.addParameter("_OS_VERSION", osVersion);

Why does this parameter name start with an underscore? It's inconsistent with other names we pass, and I don't see how it is different.

[Simon Cooper]

(In reply to comment #7)
> (From update of attachment 207224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207224&action=review
> 
> > Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.sb:325
> > +(if (equal? os-version "10.7")
> > +    (allow ipc-posix-shm)
> > +    (begin
> > +        (if (equal? os-version "10.8")
> 
> This nested code is very hard to read. 

Err really?  deeply nested?  It is an if, nested one level, on the else half of another if.  It is no different from,

    if (predicateA) {
        some stuff
    } else {
	if (predicateB) {
	    other stuff		
	}
	more stuff
    }

I should warn you that a few lines above the code you don't like there are two function definitions that use an iterator, an anonymous function, a nested if and it calls other functions.  Please don't look at it, you could have an aneurysm :-).

Somewhat seriously, almost all of the scheme used in these profiles is covered in the first 16 pages of "The Little Schemer".  The book has. pictures. and jokes.

> Preprocessor would make it so much easier, because it has an easy to use wide set of operations, and doesn't force deep nesting:
> 
> #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080 
> ...
> #else
> ...
> #endif
> 
> Also, preprocessor would have had zero cost at runtime.

Using the preprocessor has a non-zero cost for me every time and for each operating system I want to try out a profile on.  I have to preprocess them.

BTW, the scheme in question only takes about a micro-second of time to evaluate.

> 
> > Source/WebKit2/Shared/SandboxInitializationParameters.h:49
> > +    void addParameter(const char* name, NSString *value);
> 
> You probably did this for consistency with existing addPathParameter function, however it stands out to me as unnecessary to round-trip via an Objective C value in this case.

Neither ::ascii or ::latin1 are correct. 

I will change it to use .utf8().data() and pass in a c-string.

> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:173
> > +    if(osVersionParts.size() < 2) {
> 
> There should be a space between "if" and "(".

Okay.

> 
> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:180
> > +    sandboxParameters.addParameter("_OS_VERSION", osVersion);
> 
> Why does this parameter name start with an underscore? It's inconsistent with other names we pass, and I don't see how it is different.

I used _OS_VERSION because this really ought to be part of the Sandbox implementation.  It is not really for direct "use" by the profiles, but part of the internal way that "os-version" gets set in the scheme profile.

[Simon Cooper]

Created attachment 207357 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 207357 [details]
Patch

Clearing flags on attachment: 207357

Committed r153070: <http://trac.webkit.org/changeset/153070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.