WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
130879
CVE-2013-2875
Clear SVGInlineTextBox fragments when the text changes.
https://bugs.webkit.org/show_bug.cgi?id=130879
Summary
Clear SVGInlineTextBox fragments when the text changes.
Myles C. Maxfield
Reported
2014-03-27 17:47:08 PDT
Clear SVGInlineTextBox fragments when the text changes.
Attachments
Patch
(7.37 KB, patch)
2014-03-27 17:48 PDT
,
Myles C. Maxfield
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2014-03-27 17:48:38 PDT
Created
attachment 228012
[details]
Patch
Myles C. Maxfield
Comment 2
2014-03-27 17:49:14 PDT
<
rdar://problem/15695951
>
Darin Adler
Comment 3
2014-03-28 01:03:03 PDT
Comment on
attachment 228012
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=228012&action=review
> Source/WebCore/ChangeLog:16 > + Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox, which was all messed up.
I don’t understand this comment. The patch adds a few final. but I don’t see any cleanup of virtual or override. Maybe this is just a comment from Blink that makes no sense here.
> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:70 > + InlineTextBox* nextBox = nextTextBox(); > + if (nextBox)
I’d suggest putting the variable definition inside the if statement.
> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:71 > + nextBox->dirtyLineBoxes();
I’m slightly concerned that this is a recursive algorithm, which might end up using stack proportional to the number of successive SVGInlineTextBox objects. Is there a clean way to do this in an iterative fashion rather than recursive? Maybe the compiler’s tail call optimization will take care of it?
> LayoutTests/ChangeLog:16 > + This patch modifies SVGInlineTextBox::dirtyLineBoxes to clear all > + following text boxes when invoked. Typically this method is called > + when the underlying text string changes, and that change needs to > + be propagated to all the boxes that use the text beyond the point > + where the text is first modified. > + > + Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox, which was all messed up.
We should not be repeating these comments here. The comments here should just talk about the tests.
> LayoutTests/svg/custom/unicode-in-tspan-multi-svg-crash.html:36 > + <p>Test Passes if there is no crash in Debug or Asan builds. There should be no characters preceding "Test".</p>
Seems pretty weak to have a test where passing is “not crashing”. It seems to me that we could come up with a test that shows checks for correct behavior, maybe a reference test. But I guess it’s efficient to just use the test the Blink contributor provided with his patch, even though it has serious shortcomings.
Myles C. Maxfield
Comment 4
2014-03-28 10:23:47 PDT
Comment on
attachment 228012
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=228012&action=review
>> Source/WebCore/ChangeLog:16 >> + Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox, which was all messed up. > > I don’t understand this comment. The patch adds a few final. but I don’t see any cleanup of virtual or override. > > Maybe this is just a comment from Blink that makes no sense here.
It is. I'll update this comment.
>> LayoutTests/svg/custom/unicode-in-tspan-multi-svg-crash.html:36 >> + <p>Test Passes if there is no crash in Debug or Asan builds. There should be no characters preceding "Test".</p> > > Seems pretty weak to have a test where passing is “not crashing”. It seems to me that we could come up with a test that shows checks for correct behavior, maybe a reference test. > > But I guess it’s efficient to just use the test the Blink contributor provided with his patch, even though it has serious shortcomings.
Without the patch, this line doesn't get shown at all on my mac. Instead, garbage characters are shown instead. A dump-as-text test should be reasonable in this case.
Myles C. Maxfield
Comment 5
2014-03-28 11:26:12 PDT
Comment on
attachment 228012
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=228012&action=review
>>> Source/WebCore/ChangeLog:16 >>> + Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox, which was all messed up. >> >> I don’t understand this comment. The patch adds a few final. but I don’t see any cleanup of virtual or override. >> >> Maybe this is just a comment from Blink that makes no sense here. > > It is. I'll update this comment.
Done.
>> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:70 >> + if (nextBox) > > I’d suggest putting the variable definition inside the if statement.
Done.
>> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:71 >> + nextBox->dirtyLineBoxes(); > > I’m slightly concerned that this is a recursive algorithm, which might end up using stack proportional to the number of successive SVGInlineTextBox objects. Is there a clean way to do this in an iterative fashion rather than recursive? Maybe the compiler’s tail call optimization will take care of it?
Done.
>> LayoutTests/ChangeLog:16 >> + Also cleans up virtual, OVERRIDE and FINAL for SVGRootInlineBox, which was all messed up. > > We should not be repeating these comments here. The comments here should just talk about the tests.
Done.
Myles C. Maxfield
Comment 6
2014-03-28 11:26:24 PDT
http://trac.webkit.org/changeset/166420
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug