Bug 81466

Summary:

Do not copy the script source in the SourceProvider, just reference the existing string

Product:

WebKit

Reporter:

Benjamin Poulain <benjamin>

Component:

JavaScriptCore

Assignee:

Benjamin Poulain <benjamin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, jberlin, yong.li.webkit

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	ggaren: review+, buildbot: commit-queue-

Benjamin Poulain

Reported 2012-03-17 22:36:13 PDT

When executing scripts, we copy giant strings in the SourceProvider. That is not good and very simple to fix.

Attachments
Patch (5.24 KB, patch) 2012-03-17 22:40 PDT, Benjamin Poulain	ggaren: review+ buildbot: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Benjamin Poulain

Comment 1 2012-03-17 22:40:07 PDT

Created attachment 132483 [details] Patch

Alexey Proskuryakov

Comment 2 2012-03-18 20:26:15 PDT

Comment on attachment 132483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132483&action=review > Source/JavaScriptCore/parser/SourceProvider.h:89 > + UString getRange(int start, int end) const OVERRIDE If it's OVERRIDE, it also needs to be virtual. Although it can be implicit in C++, we prefer to spell it out. > Source/WebCore/bindings/js/StringSourceProvider.h:45 > + JSC::UString getRange(int start, int end) const OVERRIDE Ditto.

Build Bot

Comment 3 2012-03-19 12:29:56 PDT

Comment on attachment 132483 [details] Patch Attachment 132483 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11986329

Geoffrey Garen

Comment 4 2012-03-19 13:40:54 PDT

Comment on attachment 132483 [details] Patch r=me Please add "virtual" to these declarations, as Alexey suggested.

Benjamin Poulain

Comment 5 2012-03-19 22:27:02 PDT

Committed r111358: <http://trac.webkit.org/changeset/111358>

Jessie Berlin

Comment 6 2012-03-19 23:00:38 PDT

(In reply to comment #5) > Committed r111358: <http://trac.webkit.org/changeset/111358> This broke the Mac build: http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/7932/steps/compile-webkit/logs/stdio

Benjamin Poulain

Comment 7 2012-03-19 23:18:51 PDT

> This broke the Mac build: > > http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/7932/steps/compile-webkit/logs/stdio Damn, that is dumb. Give me a minute, I'll fix that.

Yong Li

Comment 8 2012-05-07 11:50:24 PDT

*** Bug 79125 has been marked as a duplicate of this bug. ***

Yong Li

Comment 9 2012-05-07 12:03:05 PDT

There is no test, neither can I find a reason why there is no test in here or change log.

Benjamin Poulain

Comment 10 2012-05-07 12:08:32 PDT

> There is no test, neither can I find a reason why there is no test in here or change log. Are you kidding me? How would you test this?

Yong Li

Comment 11 2012-05-07 12:11:48 PDT

(In reply to comment #10) > > There is no test, neither can I find a reason why there is no test in here or change log. > > Are you kidding me? How would you test this? I don't have a quick solution for testing it, either. But I would mention that in change log or bugzilla, as I've been told by many reviewers.

Benjamin Poulain

Comment 12 2012-05-07 12:14:59 PDT

> I don't have a quick solution for testing it, either. But I would mention that in change log or bugzilla, as I've been told by many reviewers. We also use common sense in reviews.

Note You need to log in before you can comment on or make changes to this bug.