Bug 163879

Created attachment 292588 [details] Only check the Web process if the UI process is being g debugged

Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged View in context: https://bugs.webkit.org/attachment.cgi?id=292588&action=review > Source/WebKit2/UIProcess/Cocoa/WebProcessProxyCocoa.mm:140 > + if (!processIsBeingDebugged(getpid())) > + return false; > + > + return processIsBeingDebugged(processIdentifier()); I like using short-circuit boolean algebra for this kind of thing, and I also believe this needs a why comment since the name of the function and implementation don’t match in what might be a surprising way. Something like this, only more accurate and better worded and maybe even broken into multiple lines: // Checking our own process first and then the target process means we won’t try something we don’t have permissions for in the production, sandboxed case. return processIsBeingDebugged(getpid()) && processIsBeingDebugged(processIdentifier());

Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged View in context: https://bugs.webkit.org/attachment.cgi?id=292588&action=review > Source/WebKit2/ChangeLog:11 > + (WebKit::WebProcessProxy::platformIsBeingDebugged): If the UI process is not currently being Why is this right? I almost never debug both processes, so at least as far as I'm concerned, this completely breaks the intent of the original patch. Also, why debugging the UI process signifies that it's sandbox is open?

Darin's r+ got reverted accidentally, but I think that my questions are valid.

I am not in any hurry to r+ this; I’m sure Dan thought it through and if he writes a comment like the one I requested it will likely help the rest of us understand his thinking.

Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged Thanks for your comments, Alexey. This mechanism isn’t going to be as useful as I wish it were, but I think it’s still worth keeping. I am going to write a patch that uses WebKit::processIsSandboxed() on the UI process, which is a much better way to predict if checking whether checking the state of another process will succeed.

I guess another way to predict if this will work is to perform a dry run using sandbox_check. Of course, that check may break in the future if internal implementation of this sysctl changes.

Created attachment 292720 [details] Patch

Committed <https://trac.webkit.org/r207807>.