Changeset 101556 in webkit

Timestamp:

Nov 30, 2011 2:33:20 PM (12 years ago)

Author:

rniwa@webkit.org

Message:

Assertion failure (m_nestedIsolateCount >= 1) in BidiResolver::exitIsolate()
​https://bugs.webkit.org/show_bug.cgi?id=69267

Reviewed by Eric Seidel.

Source/WebCore:

The failure was caused by our updating bidi resolver's current position in layoutRunsAndFloatsInRange
without updating the number of nested isolated ancestors. Fixed the bug by computing the number of
isolated ancestors when setting a new position to the bidi resolver.

Also renamed the existing BidiResolver::setPosition to setPositionIgnoringNestedIsolates because this
version can be used only when we don't have to update the number of nested isolates.

Tests: fast/text/bidi-isolate-hang-with-neutral-expected.html

fast/text/bidi-isolate-hang-with-neutral.html
fast/text/bidi-isolate-nextlinebreak-failure.html

• platform/graphics/GraphicsContext.cpp:

(WebCore::GraphicsContext::drawBidiText):

• platform/text/BidiResolver.h:

(WebCore::BidiResolver::setPositionIgnoringNestedIsolates):
(WebCore::BidiResolver::setPosition):

• rendering/InlineIterator.h:

(WebCore::numberOfIsolateAncestors): Takes InlineIterator instead of object and root.
(WebCore::InlineBidiResolver::appendRun):

• rendering/RenderBlockLineLayout.cpp:

(WebCore::constructBidiRuns):
(WebCore::RenderBlock::layoutRunsAndFloatsInRange):
(WebCore::RenderBlock::determineStartPosition):

LayoutTests:

Add a regression test for the assertion failure. Also add a regression test for a hang
found by Levi Weintraub and Jeremy Moskovich.

This patch also fixes the assertion failure in fast/block/child-not-removed-from-parent-lineboxes-crash.html
introduced by r101268.

• fast/text/bidi-isolate-hang-with-neutral-expected.html: Added.
• fast/text/bidi-isolate-hang-with-neutral.html: Added.
• fast/text/bidi-isolate-nextlinebreak-failure-expected.txt: Added.
• fast/text/bidi-isolate-nextlinebreak-failure.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/bidi-isolate-hang-with-neutral-expected.html (added)
• LayoutTests/fast/text/bidi-isolate-hang-with-neutral.html (added)
• LayoutTests/fast/text/bidi-isolate-nextlinebreak-failure-expected.txt (added)
• LayoutTests/fast/text/bidi-isolate-nextlinebreak-failure.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/GraphicsContext.cpp (modified) (1 diff)
• Source/WebCore/platform/text/BidiResolver.h (modified) (1 diff)
• Source/WebCore/rendering/InlineIterator.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderBlockLineLayout.cpp (modified) (6 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-11-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assertion failure (m_nestedIsolateCount >= 1) in BidiResolver::exitIsolate()
+        https://bugs.webkit.org/show_bug.cgi?id=69267
+
+        Reviewed by Eric Seidel.
+
+        Add a regression test for the assertion failure. Also add a regression test for a hang
+        found by Levi Weintraub and Jeremy Moskovich.
+
+        This patch also fixes the assertion failure in fast/block/child-not-removed-from-parent-lineboxes-crash.html
+        introduced by r101268.
+
+        * fast/text/bidi-isolate-hang-with-neutral-expected.html: Added.
+        * fast/text/bidi-isolate-hang-with-neutral.html: Added.
+        * fast/text/bidi-isolate-nextlinebreak-failure-expected.txt: Added.
+        * fast/text/bidi-isolate-nextlinebreak-failure.html: Added.
+
 2011-11-30  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-11-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assertion failure (m_nestedIsolateCount >= 1) in BidiResolver::exitIsolate()
+        https://bugs.webkit.org/show_bug.cgi?id=69267
+
+        Reviewed by Eric Seidel.
+
+        The failure was caused by our updating bidi resolver's current position in layoutRunsAndFloatsInRange
+        without updating the number of nested isolated ancestors. Fixed the bug by computing the number of
+        isolated ancestors when setting a new position to the bidi resolver.
+
+        Also renamed the existing BidiResolver::setPosition to setPositionIgnoringNestedIsolates because this
+        version can be used only when we don't have to update the number of nested isolates.
+
+        Tests: fast/text/bidi-isolate-hang-with-neutral-expected.html
+               fast/text/bidi-isolate-hang-with-neutral.html
+               fast/text/bidi-isolate-nextlinebreak-failure.html
+
+        * platform/graphics/GraphicsContext.cpp:
+        (WebCore::GraphicsContext::drawBidiText):
+        * platform/text/BidiResolver.h:
+        (WebCore::BidiResolver::setPositionIgnoringNestedIsolates):
+        (WebCore::BidiResolver::setPosition):
+        * rendering/InlineIterator.h:
+        (WebCore::numberOfIsolateAncestors): Takes InlineIterator instead of object and root.
+        (WebCore::InlineBidiResolver::appendRun):
+        * rendering/RenderBlockLineLayout.cpp:
+        (WebCore::constructBidiRuns):
+        (WebCore::RenderBlock::layoutRunsAndFloatsInRange):
+        (WebCore::RenderBlock::determineStartPosition):
+
 2011-11-30  Brent Fulgham  <bfulgham@webkit.org>
 

trunk/Source/WebCore/platform/graphics/GraphicsContext.cpp

@@ -401 +401 @@
     BidiResolver<TextRunIterator, BidiCharacterRun> bidiResolver;
     bidiResolver.setStatus(BidiStatus(run.direction(), run.directionalOverride()));
-    bidiResolver.setPosition(TextRunIterator(&run, 0));
+    bidiResolver.setPositionIgnoringNestedIsolates(TextRunIterator(&run, 0));
 
     // FIXME: This ownership should be reversed. We should pass BidiRunList

trunk/Source/WebCore/platform/text/BidiResolver.h

@@ -178 +178 @@
 
     const Iterator& position() const { return m_current; }
-    void setPosition(const Iterator& position) { m_current = position; }
+    void setPositionIgnoringNestedIsolates(const Iterator& position) { m_current = position; }
+    void setPosition(const Iterator& position, unsigned nestedIsolatedCount)
+    {
+        m_current = position;
+        m_nestedIsolateCount = nestedIsolatedCount;
+    }
 
     void increment() { m_current.increment(); }

trunk/Source/WebCore/rendering/InlineIterator.h

@@ -407 +407 @@
 }
 
-static inline unsigned numberOfIsolateAncestors(RenderObject* object, RenderObject* root)
-{
-    ASSERT(object);
+static inline unsigned numberOfIsolateAncestors(const InlineIterator& iter)
+{
+    RenderObject* object = iter.object();
+    if (!object)
+        return 0;
     unsigned count = 0;
-    while (object && object != root) {
+    while (object && object != iter.root()) {
         if (isIsolatedInline(object))
             count++;
@@ -483 +485 @@
         // Initialize our state depending on if we're starting in the middle of such an inline.
         // FIXME: Could this initialize from this->inIsolate() instead of walking up the render tree?
-        IsolateTracker isolateTracker(numberOfIsolateAncestors(m_sor.m_obj, m_sor.root()));
+        IsolateTracker isolateTracker(numberOfIsolateAncestors(m_sor));
         int start = m_sor.m_pos;
         RenderObject* obj = m_sor.m_obj;

trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

@@ -967 +967 @@
         if (!startObj)
             continue;
-        isolatedResolver.setPosition(InlineIterator(isolatedSpan, startObj, 0));
+        isolatedResolver.setPositionIgnoringNestedIsolates(InlineIterator(isolatedSpan, startObj, 0));
 
         // FIXME: isolatedEnd should probably equal end or the last char in isolatedSpan.
@@ -1217 +1217 @@
         layoutState.lineInfo().setEmpty(true);
 
-        InlineIterator oldEnd = end;
+        const InlineIterator oldEnd = end;
         bool isNewUBAParagraph = layoutState.lineInfo().previousLineBrokeCleanly();
         FloatingObject* lastFloatFromPreviousLine = (m_floatingObjects && !m_floatingObjects->set().isEmpty()) ? m_floatingObjects->set().last() : 0;
@@ -1285 +1285 @@
                             removeFloatingObjectsBelow(lastFloatFromPreviousLine, oldLogicalHeight);
                             setLogicalHeight(oldLogicalHeight + adjustment);
-                            resolver.setPosition(oldEnd);
+                            resolver.setPositionIgnoringNestedIsolates(oldEnd);
                             end = oldEnd;
                             continue;
@@ -1325 +1325 @@
 
         lineMidpointState.reset();
-        resolver.setPosition(end);
+        resolver.setPosition(end, numberOfIsolateAncestors(end));
     }
 }
@@ -1640 +1640 @@
     if (last) {
         setLogicalHeight(last->lineBottomWithLeading());
-        resolver.setPosition(InlineIterator(this, last->lineBreakObj(), last->lineBreakPos()));
+        InlineIterator iter = InlineIterator(this, last->lineBreakObj(), last->lineBreakPos());
+        resolver.setPosition(iter, numberOfIsolateAncestors(iter));
         resolver.setStatus(last->lineBreakBidiStatus());
     } else {
@@ -1649 +1650 @@
         }
         resolver.setStatus(BidiStatus(direction, style()->unicodeBidi() == Override));
-        resolver.setPosition(InlineIterator(this, bidiFirstSkippingEmptyInlines(this, &resolver), 0));
+        InlineIterator iter = InlineIterator(this, bidiFirstSkippingEmptyInlines(this, &resolver), 0);
+        resolver.setPosition(iter, numberOfIsolateAncestors(iter));
     }
     return curr;