ScriptSourceCode::source assumes the provider for the source code of a script is of type StringSourceProvider. However, if the source code is from a cached script, then the provider is CachedScriptSourceProvider. Hence, unexpected behavior and/or a crash can follow if you call ScriptSourceCode::source on a cached script.
You can reproducibly cause a crash by adding the following line above line 87 in ScriptController.cpp (http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp#L87): printf("%s\n", sourceCode.source().ascii().data()); Then run layout test /http/tests/security/xssAuditor/base-href.html
Created attachment 33165 [details] Patch Created a new base class called ScriptSourceProvider which is inherited by both StringSourceProvider and CachedScriptSourceProvider. Blindly edited the WebCore Visual C++ project file.
Comment on attachment 33165 [details] Patch The code looks good, but when you add a file to the project, you should really add it to all the build systems. I think you've missed WebCore.gypi for example. You should find "ScriptSourceCode.h" and make sure your new file shows up in all the same places.
Created attachment 33230 [details] Patch Added ScriptSourceProvider to WebCore.gypi.
Comment on attachment 33230 [details] Patch There are yet more build systems in the tree. Yes it sucks. For example: http://trac.webkit.org/browser/trunk/WebCore/WebCore.pro Also, can we test for this crash in a LayoutTest? If it involves a cached script, we might need to load the same script twice.
Created attachment 33239 [details] Patch Added bindings/js/ScriptSourceProvider.h, ScriptSourceCode.h, StringSourceProvider.h to WebCore.pro. I hope I've got all the build systems covered.
It does not appear that anybody (besides ScriptController.cpp) calls ScriptSourceCode::source. So, I don't believe we can test this crash in a LayoutTest alone. (In reply to comment #5) > (From update of attachment 33230 [details]) > There are yet more build systems in the tree. Yes it sucks. For example: > > http://trac.webkit.org/browser/trunk/WebCore/WebCore.pro > > Also, can we test for this crash in a LayoutTest? If it involves a cached > script, we might need to load the same script twice.
Comment on attachment 33239 [details] Patch Confirmed with Dan on IRC that this crash only surfaces after he makes another change he's working on. It's a crash fix from the future!
Created attachment 33245 [details] Patch I hope this fixes the Xcode issue.
Created attachment 33246 [details] Patch Oops. Forgot the change log last time.
Comment on attachment 33246 [details] Patch Ok. Let's try it.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/WebCore.pro M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/js/CachedScriptSourceProvider.h M WebCore/bindings/js/ScriptSourceCode.h A WebCore/bindings/js/ScriptSourceProvider.h M WebCore/bindings/js/StringSourceProvider.h Committed r46216 M WebCore/WebCore.pro M WebCore/ChangeLog M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/bindings/js/StringSourceProvider.h M WebCore/bindings/js/CachedScriptSourceProvider.h M WebCore/bindings/js/ScriptSourceCode.h A WebCore/bindings/js/ScriptSourceProvider.h M WebCore/WebCore.xcodeproj/project.pbxproj r46216 = 2e428bb4856f165dba70c85ccd5666e79af5d6bf (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46216
+ virtual ~ScriptSourceProvider() { } There is no need to explicitly define an empty inline destructor here - the compiler would generate an identical one. It could make sense to define it as non-inline though, to reduce code bloat.