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
Patch v2 - Remove that second set of guards, also. (4.05 KB, patch)
2011-05-18 08:48 PDT, Brady Eidson
beidson: review-
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-
Patch v4 - Anders' feedback (15.71 KB, patch)
2011-05-18 11:39 PDT, Brady Eidson
no flags
Patch v5 - Anders wanted a rewrite using c-strings (17.68 KB, patch)
2011-05-18 13:27 PDT, Brady Eidson
andersca: review+
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
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.