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

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