Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 93855 [details]
Patch v1 - Strip the shim dylibs from the environment variable

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.

[Brady Eidson]

Created attachment 93922 [details]
Patch v2 - Remove that second set of guards, also.

[Brady Eidson]

Created attachment 93941 [details]
Patch v3 - Better, shared code for sanitizing the variables and do it in PluginProcessMain also.

[Anders Carlsson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 93955 [details]
Patch v4 - Anders' feedback

[Brady Eidson]

Created attachment 93977 [details]
Patch v5 - Anders wanted a rewrite using c-strings

[WebKit Review Bot]

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.

[Anders Carlsson]

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.

[Brady Eidson]

(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

[Brady Eidson]

Nuked the #include r86797

[Ryosuke Niwa]

It seems like this patch broke waterfall :(
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11788

[Brady Eidson]

(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.