- Revision
- 102875
- Author
- [email protected]
- Date
- 2011-12-14 20:19:24 -0800 (Wed, 14 Dec 2011)
Log Message
Crash due to incorrect parsing of isolates
https://bugs.webkit.org/show_bug.cgi?id=74311
When <bdi> content is wrapped, all hell breaks loose
https://bugs.webkit.org/show_bug.cgi?id=74396
Patch by Ken Buchanan <[email protected]> on 2011-12-14
Reviewed by Eric Seidel.
Source/WebCore:
When an isolate was encountered during run layout, the entire isolate
would be parsed, even if the run started in the middle of the isolate.
This would sometimes cause parts of the isolate to be added multiple
times as runs. This patch marks the starting position within the
isolate so nothing is parsed twice.
This patch changes appendRun() so that when a run is added that is
inside an isolate, it saves the start position of the run rather than
the root. This allows constructBidiRuns() to resume parsing the
isolate from the correct position.
The change to RenderBox partially reverts a previous change I had
done. It makes sense to screen for the condition, as was previously
the case.
This patch does not add the test case from 74396 because a separate
bug is preventing it from rendering correctly.
* rendering/InlineIterator.h:
(WebCore::addPlaceholderRunForIsolatedInline)
(WebCore::IsolateTracker::addFakeRunIfNecessary)
(WebCore::InlineBidiResolver::appendRun)
* rendering/RenderBlockLineLayout.cpp:
(WebCore::RenderBlockLineLayout::constructBidiRuns)
* rendering/RenderBox.cpp:
(WebCore::RenderBox::positionLineBox)
LayoutTests:
Layout test has multiple lines inside an isolate to ensure each run
is created only once.
The test that is in bug 74396 will have to be added once bug 74489 is
fixed.
* LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt: Added
* LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html: Added
Modified Paths
Added Paths
Property Changed
Diff
Modified: trunk/LayoutTests/ChangeLog (102874 => 102875)
--- trunk/LayoutTests/ChangeLog 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/LayoutTests/ChangeLog 2011-12-15 04:19:24 UTC (rev 102875)
@@ -1,3 +1,21 @@
+2011-12-14 Ken Buchanan <[email protected]>
+
+ Crash due to incorrect parsing of isolates
+ https://bugs.webkit.org/show_bug.cgi?id=74311
+ When <bdi> content is wrapped, all hell breaks loose
+ https://bugs.webkit.org/show_bug.cgi?id=74396
+
+ Reviewed by Eric Seidel.
+
+ Layout test has multiple lines inside an isolate to ensure each run
+ is created only once.
+
+ The test that is in bug 74396 will have to be added once bug 74489 is
+ fixed.
+
+ * LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt: Added
+ * LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html: Added
+
2011-12-14 Jing Zhao <[email protected]>
Opening two popup menus by dispatchEvent() makes problems.
Property changes on: trunk/LayoutTests/ChangeLog
___________________________________________________________________
Added: svn:executable
Modified: trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash-expected.txt (102874 => 102875)
--- trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash-expected.txt 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash-expected.txt 2011-12-15 04:19:24 UTC (rev 102875)
@@ -1 +1 @@
- PASS, if no exception or crash in debug
+PASS, if no exception or crash in debug
Added: trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt (0 => 102875)
--- trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt 2011-12-15 04:19:24 UTC (rev 102875)
@@ -0,0 +1,7 @@
+
+ PASS if this text displays only once, and no exception or crash
+
+
+
+
+
Property changes on: trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash-expected.txt
___________________________________________________________________
Added: svn:executable
Added: trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html (0 => 102875)
--- trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html (rev 0)
+++ trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html 2011-12-15 04:19:24 UTC (rev 102875)
@@ -0,0 +1,12 @@
+<pre>
+ <bdi>
+ PASS if this text displays only once, and no exception or crash
+ <object></object>
+ <div></div>
+ </bdi>
+</pre>
+
+<script>
+if (window.layoutTestController)
+ layoutTestController.dumpAsText();
+</script>
Property changes on: trunk/LayoutTests/fast/text/international/multiline-and-object-inside-isolate-crash.html
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/ChangeLog (102874 => 102875)
--- trunk/Source/WebCore/ChangeLog 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/Source/WebCore/ChangeLog 2011-12-15 04:19:24 UTC (rev 102875)
@@ -1,3 +1,39 @@
+2011-12-14 Ken Buchanan <[email protected]>
+
+ Crash due to incorrect parsing of isolates
+ https://bugs.webkit.org/show_bug.cgi?id=74311
+ When <bdi> content is wrapped, all hell breaks loose
+ https://bugs.webkit.org/show_bug.cgi?id=74396
+
+ Reviewed by Eric Seidel.
+
+ When an isolate was encountered during run layout, the entire isolate
+ would be parsed, even if the run started in the middle of the isolate.
+ This would sometimes cause parts of the isolate to be added multiple
+ times as runs. This patch marks the starting position within the
+ isolate so nothing is parsed twice.
+
+ This patch changes appendRun() so that when a run is added that is
+ inside an isolate, it saves the start position of the run rather than
+ the root. This allows constructBidiRuns() to resume parsing the
+ isolate from the correct position.
+
+ The change to RenderBox partially reverts a previous change I had
+ done. It makes sense to screen for the condition, as was previously
+ the case.
+
+ This patch does not add the test case from 74396 because a separate
+ bug is preventing it from rendering correctly.
+
+ * rendering/InlineIterator.h:
+ (WebCore::addPlaceholderRunForIsolatedInline)
+ (WebCore::IsolateTracker::addFakeRunIfNecessary)
+ (WebCore::InlineBidiResolver::appendRun)
+ * rendering/RenderBlockLineLayout.cpp:
+ (WebCore::RenderBlockLineLayout::constructBidiRuns)
+ * rendering/RenderBox.cpp:
+ (WebCore::RenderBox::positionLineBox)
+
2011-12-14 Jing Zhao <[email protected]>
Opening two popup menus by dispatchEvent() makes problems.
Property changes on: trunk/Source/WebCore/ChangeLog
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/rendering/InlineIterator.h (102874 => 102875)
--- trunk/Source/WebCore/rendering/InlineIterator.h 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/Source/WebCore/rendering/InlineIterator.h 2011-12-15 04:19:24 UTC (rev 102875)
@@ -422,10 +422,10 @@
// FIXME: This belongs on InlineBidiResolver, except it's a template specialization
// of BidiResolver which knows nothing about RenderObjects.
-static inline void addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolver, RenderObject* isolatedInline)
+static inline void addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolver, RenderObject* obj, unsigned pos)
{
- ASSERT(isolatedInline);
- BidiRun* isolatedRun = new (isolatedInline->renderArena()) BidiRun(0, 0, isolatedInline, resolver.context(), resolver.dir());
+ ASSERT(obj);
+ BidiRun* isolatedRun = new (obj->renderArena()) BidiRun(pos, 0, obj, resolver.context(), resolver.dir());
resolver.runs().addRun(isolatedRun);
// FIXME: isolatedRuns() could be a hash of object->run and then we could cheaply
// ASSERT here that we didn't create multiple objects for the same inline.
@@ -453,23 +453,17 @@
// We don't care if we encounter bidi directional overrides.
void embed(WTF::Unicode::Direction, BidiEmbeddingSource) { }
- void addFakeRunIfNecessary(RenderObject* obj, InlineBidiResolver& resolver)
+ void addFakeRunIfNecessary(RenderObject* obj, unsigned pos, InlineBidiResolver& resolver)
{
- // We only need to lookup the root isolated span and add a fake run
- // for it once, but we'll be called for every span inside the isolated span
- // so we just ignore the call.
+ // We only need to add a fake run for a given isolated span once during each call to createBidiRunsForLine.
+ // We'll be called for every span inside the isolated span so we just ignore subsequent calls.
if (m_haveAddedFakeRunForRootIsolate)
return;
m_haveAddedFakeRunForRootIsolate = true;
-
- // FIXME: position() could be outside the run and may be the wrong call here.
- // If we were passed an InlineIterator instead that would have the right root().
- RenderObject* isolatedInline = containingIsolate(obj, resolver.position().root());
- // FIXME: Because enterIsolate is not passed a RenderObject, we have to crawl up the
- // tree to see which parent inline is the isolate. We could change enterIsolate
- // to take a RenderObject and do this logic there, but that would be a layering
- // violation for BidiResolver (which knows nothing about RenderObject).
- addPlaceholderRunForIsolatedInline(resolver, isolatedInline);
+ // obj and pos together denote a single position in the inline, from which the parsing of the isolate will start.
+ // We don't need to mark the end of the run because this is implicit: it is either endOfLine or the end of the
+ // isolate, when we call createBidiRunsForLine it will stop at whichever comes first.
+ addPlaceholderRunForIsolatedInline(resolver, obj, pos);
}
private:
@@ -489,7 +483,7 @@
RenderObject* obj = m_sor.m_obj;
while (obj && obj != m_eor.m_obj && obj != endOfLine.m_obj) {
if (isolateTracker.inIsolate())
- isolateTracker.addFakeRunIfNecessary(obj, *this);
+ isolateTracker.addFakeRunIfNecessary(obj, start, *this);
else
RenderBlock::appendRunsForObject(m_runs, start, obj->length(), obj, *this);
// FIXME: start/obj should be an InlineIterator instead of two separate variables.
@@ -505,7 +499,7 @@
// It's OK to add runs for zero-length RenderObjects, just don't make the run larger than it should be
int end = obj->length() ? pos + 1 : 0;
if (isolateTracker.inIsolate())
- isolateTracker.addFakeRunIfNecessary(obj, *this);
+ isolateTracker.addFakeRunIfNecessary(obj, start, *this);
else
RenderBlock::appendRunsForObject(m_runs, start, end, obj, *this);
}
Property changes on: trunk/Source/WebCore/rendering/InlineIterator.h
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp (102874 => 102875)
--- trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp 2011-12-15 04:19:24 UTC (rev 102875)
@@ -949,6 +949,7 @@
// FIXME: We should pass a BidiRunList into createBidiRunsForLine instead
// of the resolver owning the runs.
ASSERT(&topResolver.runs() == &bidiRuns);
+ RenderObject* currentRoot = topResolver.position().root();
topResolver.createBidiRunsForLine(endOfLine, override, previousLineBrokeCleanly);
while (!topResolver.isolatedRuns().isEmpty()) {
@@ -956,23 +957,31 @@
BidiRun* isolatedRun = topResolver.isolatedRuns().last();
topResolver.isolatedRuns().removeLast();
+ RenderObject* startObj = isolatedRun->object();
+
// Only inlines make sense with unicode-bidi: isolate (blocks are already isolated).
- RenderInline* isolatedSpan = toRenderInline(isolatedRun->object());
+ // FIXME: Because enterIsolate is not passed a RenderObject, we have to crawl up the
+ // tree to see which parent inline is the isolate. We could change enterIsolate
+ // to take a RenderObject and do this logic there, but that would be a layering
+ // violation for BidiResolver (which knows nothing about RenderObject).
+ RenderInline* isolatedSpan = toRenderInline(containingIsolate(startObj, currentRoot));
InlineBidiResolver isolatedResolver;
isolatedResolver.setStatus(statusWithDirection(isolatedSpan->style()->direction()));
// FIXME: The fact that we have to construct an Iterator here
// currently prevents this code from moving into BidiResolver.
- RenderObject* startObj = bidiFirstSkippingEmptyInlines(isolatedSpan, &isolatedResolver);
- if (!startObj)
+ if (!bidiFirstSkippingEmptyInlines(isolatedSpan, &isolatedResolver))
continue;
- isolatedResolver.setPositionIgnoringNestedIsolates(InlineIterator(isolatedSpan, startObj, 0));
+ // The starting position is the beginning of the first run within the isolate that was identified
+ // during the earlier call to createBidiRunsForLine. This can be but is not necessarily the
+ // first run within the isolate.
+ InlineIterator iter = InlineIterator(isolatedSpan, startObj, isolatedRun->m_start);
+ isolatedResolver.setPositionIgnoringNestedIsolates(iter);
- // FIXME: isolatedEnd should probably equal end or the last char in isolatedSpan.
- InlineIterator isolatedEnd = endOfLine;
+ // We stop at the next end of line; we may re-enter this isolate in the next call to constructBidiRuns().
// FIXME: What should end and previousLineBrokeCleanly be?
// rniwa says previousLineBrokeCleanly is just a WinIE hack and could always be false here?
- isolatedResolver.createBidiRunsForLine(isolatedEnd, NoVisualOverride, previousLineBrokeCleanly);
+ isolatedResolver.createBidiRunsForLine(endOfLine, NoVisualOverride, previousLineBrokeCleanly);
// Note that we do not delete the runs from the resolver.
bidiRuns.replaceRunWithRuns(isolatedRun, isolatedResolver.runs());
Property changes on: trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/rendering/RenderBox.cpp (102874 => 102875)
--- trunk/Source/WebCore/rendering/RenderBox.cpp 2011-12-15 04:16:25 UTC (rev 102874)
+++ trunk/Source/WebCore/rendering/RenderBox.cpp 2011-12-15 04:19:24 UTC (rev 102875)
@@ -1477,7 +1477,10 @@
box->destroy(renderArena());
} else if (isReplaced()) {
setLocation(roundedLayoutPoint(box->topLeft()));
+ // m_inlineBoxWrapper should already be 0. Deleting it is a safeguard against security issues.
ASSERT(!m_inlineBoxWrapper);
+ if (m_inlineBoxWrapper)
+ deleteLineBoxWrapper();
m_inlineBoxWrapper = box;
}
}
