Bug 61009 - Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims
Summary: Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-17 18:16 PDT by Brady Eidson
Modified: 2011-06-18 11:55 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 - Strip the shim dylibs from the environment variable (3.61 KB, patch)
2011-05-17 18:21 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Remove that second set of guards, also. (4.05 KB, patch)
2011-05-18 08:48 PDT, Brady Eidson
beidson: review-
Details | Formatted Diff | Diff
Patch v3 - Better, shared code for sanitizing the variables and do it in PluginProcessMain also. (15.47 KB, patch)
2011-05-18 11:13 PDT, Brady Eidson
andersca: review-
Details | Formatted Diff | Diff
Patch v4 - Anders' feedback (15.71 KB, patch)
2011-05-18 11:39 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v5 - Anders wanted a rewrite using c-strings (17.68 KB, patch)
2011-05-18 13:27 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2011-05-17 18:16:31 PDT
Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims

This causes all processes spawned by WebProcess to crash, not finding the shim dylib.

In radar as <rdar://problem/9457633>
Comment 1 Brady Eidson 2011-05-17 18:21:07 PDT
Created attachment 93855 [details]
Patch v1 - Strip the shim dylibs from the environment variable
Comment 2 mitz 2011-05-17 21:46:32 PDT
Comment on attachment 93855 [details]
Patch v1 - Strip the shim dylibs from the environment variable

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

> Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:-148
> -#ifndef BUILDING_ON_SNOW_LEOPARD

You will now need to also remove the Snow Leopard guards added in r86734 to WebProcessMainMac.mm.
Comment 3 Brady Eidson 2011-05-18 08:48:48 PDT
Created attachment 93922 [details]
Patch v2 - Remove that second set of guards, also.
Comment 4 Brady Eidson 2011-05-18 11:13:50 PDT
Created attachment 93941 [details]
Patch v3 - Better, shared code for sanitizing the variables and do it in PluginProcessMain also.
Comment 5 Anders Carlsson 2011-05-18 11:21:02 PDT
Comment on attachment 93941 [details]
Patch v3 - Better, shared code for sanitizing the variables and do it in PluginProcessMain also.

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

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:38
> +    const char* environmentVariableBuffer = environmentVariable.utf8().data();

This is a dangling pointer. You need to declare a CString.

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:39
> +    String environmentValue = String::fromUTF8(getenv(environmentVariableBuffer));

I'd assign getenv to a pointer and check for null before calling String::fromUTF8.

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:49
> +    size_t count = values.size();
> +    for (size_t i = 0; i < count; ++i) {

In WebKit2 we usually write this

for (size_t i = 0, count = values.size(); i < count; ++i)

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:63
> +    // If the builder's length is 1 longer than the original environment variable, that 1 character
> +    // is the unneeded trailing ":" and the actual values are the same, so we're done.
> +    if (builder.length() == environmentValue.length() + 1)
> +        return;

Could you assert that this is the case?

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:68
> +    char* newValueBuffer = const_cast<char*>(newValueCString.data());

I think you can use mutableData here, instead of doing a const_cast.
Comment 6 Brady Eidson 2011-05-18 11:30:09 PDT
(In reply to comment #5)
> (From update of attachment 93941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93941&action=review
> 
> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:38
> > +    const char* environmentVariableBuffer = environmentVariable.utf8().data();
> 
> This is a dangling pointer. You need to declare a CString.

Yikes, yup.

> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:39
> > +    String environmentValue = String::fromUTF8(getenv(environmentVariableBuffer));
> 
> I'd assign getenv to a pointer and check for null before calling String::fromUTF8.

Okay.

> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:49
> > +    size_t count = values.size();
> > +    for (size_t i = 0; i < count; ++i) {
> 
> In WebKit2 we usually write this
> 
> for (size_t i = 0, count = values.size(); i < count; ++i)

Sweet.

> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:63
> > +    // If the builder's length is 1 longer than the original environment variable, that 1 character
> > +    // is the unneeded trailing ":" and the actual values are the same, so we're done.
> > +    if (builder.length() == environmentValue.length() + 1)
> > +        return;
> 
> Could you assert that this is the case?

Yup.

> > Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:68
> > +    char* newValueBuffer = const_cast<char*>(newValueCString.data());
> 
> I think you can use mutableData here, instead of doing a const_cast.

I thought mutableData() would always make a copy.  Apparently it doesn't!  Will-do.
Comment 7 Brady Eidson 2011-05-18 11:39:13 PDT
Created attachment 93955 [details]
Patch v4 - Anders' feedback
Comment 8 Brady Eidson 2011-05-18 13:27:27 PDT
Created attachment 93977 [details]
Patch v5 - Anders wanted a rewrite using c-strings
Comment 9 WebKit Review Bot 2011-05-18 13:28:57 PDT
Attachment 93977 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:65:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:65:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:66:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:66:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:90:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:90:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Anders Carlsson 2011-05-18 13:35:55 PDT
Comment on attachment 93977 [details]
Patch v5 - Anders wanted a rewrite using c-strings

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

> Source/WebKit2/Platform/unix/EnvironmentUtilities.h:35
> +void stripValuesEndingWithString(const String& environmentVariable, const String& value);

These could take CStrings or char*, it's up to you :)

> Source/WebKit2/WebProcess/mac/WebProcessMainMac.mm:46
> +#import <wtf/text/StringBuilder.h>

Not needed.
Comment 11 Brady Eidson 2011-05-18 15:03:34 PDT
(In reply to comment #10)

> > Source/WebKit2/WebProcess/mac/WebProcessMainMac.mm:46
> > +#import <wtf/text/StringBuilder.h>
> 
> Not needed.

Whoops, forgot to nuke this one.  Will followup.

Mostly landed in r86792
Comment 12 Brady Eidson 2011-05-18 15:05:17 PDT
Nuked the #include r86797
Comment 13 Ryosuke Niwa 2011-05-18 18:49:49 PDT
It seems like this patch broke waterfall :(
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11788
Comment 14 Brady Eidson 2011-05-18 19:30:17 PDT
(In reply to comment #13)
> It seems like this patch broke waterfall :(
> http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11788

Never ending comedy of errors, this one...  *sigh*

Disabled again in 86814

I'll make sure both basic browsing and the video layouttests work on SnowLeopard before reenabling.