Bug 26555 - Fix Chromium canary bot
Summary: Fix Chromium canary bot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-19 11:54 PDT by Adam Barth
Modified: 2009-06-19 16:44 PDT (History)
4 users (show)

See Also:

Attachments
patch (4.15 KB, patch)
2009-06-19 12:23 PDT, Adam Barth 		dglazkov: review+
Details | Formatted Diff | Diff
work-in-progress patch (807 bytes, patch)
2009-06-19 16:35 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-06-19 11:54:45 PDT 
The Chromium canary bot is unhappy with some parts of the XSSAuditor.  Patch forthcoming.
Comment 1 Adam Barth 2009-06-19 12:23:48 PDT 
Created attachment 31559 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-06-19 13:01:57 PDT 
Comment on attachment 31559 [details]
patch

yay!
Comment 3 Adam Barth 2009-06-19 13:21:08 PDT 
Will land.  DRT is chugging along as we speak.
Comment 4 Adam Barth 2009-06-19 13:40:44 PDT 
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/ScriptController.cpp
Sending        WebCore/bindings/js/ScriptSourceCode.h
Sending        WebCore/bindings/v8/ScriptController.cpp
Sending        WebCore/page/XSSAuditor.cpp
Sending        WebCore/page/XSSAuditor.h
Transmitting file data ......
Committed revision 44869.
Comment 5 Darin Adler 2009-06-19 16:00:51 PDT 
The source() function in ScriptSourceCode.h looks bad to me. It looks like JavaScriptCore is being forced to copy some code -- won't that make things slow?
Comment 6 David Levin 2009-06-19 16:12:42 PDT 
I agree with Darin.

It looks like that copy was in there before *but* it was only done when m_isEnabled was true.

59  bool XSSAuditor::canEvaluate(const ScriptSourceCode& sourceCode) const
60  {
61      if (!m_isEnabled)
62          return true;
63      
64      return canEvaluate(String(sourceCode.jsSourceCode().data(), sourceCode.jsSourceCode().length()));

A simple fix to restore old behavior would be to change this line
   84     if (!m_XSSAuditor->canEvaluate(sourceCode.source())) {
to 
 84     if (m_XSSAuditor->isEnabled() && !m_XSSAuditor->canEvaluate(sourceCode.source())) {
Comment 7 Adam Barth 2009-06-19 16:19:56 PDT 
(In reply to comment #5)
> The source() function in ScriptSourceCode.h looks bad to me. It looks like
> JavaScriptCore is being forced to copy some code -- won't that make things
> slow?

Maybe ScriptSourceCode should just grab a reference to the string on construction?  It looks like the string is kept alive anyway because the ScriptSourceCode holds a JSC::SourceCode which holds a RefPtr<SourceProvider> which holds String m_source (via StringSourceProvider : public JSC::SourceProvider).
Comment 8 Adam Barth 2009-06-19 16:35:34 PDT 
Created attachment 31572 [details]
work-in-progress patch
Comment 9 Adam Barth 2009-06-19 16:44:27 PDT 
Follow patch in https://bugs.webkit.org/show_bug.cgi?id=26561