WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61009
Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims
https://bugs.webkit.org/show_bug.cgi?id=61009
Summary
Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims
Brady Eidson
Reported
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
>
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2011-05-17 18:21:07 PDT
Created
attachment 93855
[details]
Patch v1 - Strip the shim dylibs from the environment variable
mitz
Comment 2
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.
Brady Eidson
Comment 3
2011-05-18 08:48:48 PDT
Created
attachment 93922
[details]
Patch v2 - Remove that second set of guards, also.
Brady Eidson
Comment 4
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.
Anders Carlsson
Comment 5
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.
Brady Eidson
Comment 6
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.
Brady Eidson
Comment 7
2011-05-18 11:39:13 PDT
Created
attachment 93955
[details]
Patch v4 - Anders' feedback
Brady Eidson
Comment 8
2011-05-18 13:27:27 PDT
Created
attachment 93977
[details]
Patch v5 - Anders wanted a rewrite using c-strings
WebKit Review Bot
Comment 9
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.
Anders Carlsson
Comment 10
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.
Brady Eidson
Comment 11
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
Brady Eidson
Comment 12
2011-05-18 15:05:17 PDT
Nuked the #include
r86797
Ryosuke Niwa
Comment 13
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
Brady Eidson
Comment 14
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug