Bug 88395 – Null-pointer crash in InlineFlowBox::computeOverAnnotationAdjustment during rendering/reload race

• Status: ASSIGNED
• Component: Layout and Rendering

As reported in http://code.google.com/p/chromium/issues/detail?id=128904

curr->renderer()->style(isFirstLineStyle()) can return null

Created attachment 145948 [details]
Patch

The included layout test is flaky, it will crash in Chrome almost immediately, in Safari after a few hundred cycles, and rarely under the webkit test runner.

The patch avoids the specific problem demonstrated by the test, however it is likely not the correct solution to the deeper problem.

[WebKit Review Bot]

Comment on attachment 145948 [details]
Patch

Attachment 145948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12901940

New failing tests:
fast/text/firstline/crash-firstline-detach-reload.html

[WebKit Review Bot]

Created attachment 146107 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Chang]

Comment on attachment 145948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145948&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:1446
> -            if (style->textEmphasisMark() != TextEmphasisMarkNone && toInlineTextBox(curr)->getEmphasisMarkPosition(style, emphasisMarkPosition) && emphasisMarkPosition == TextEmphasisPositionOver) {
> +            if (style && style->textEmphasisMark() != TextEmphasisMarkNone && toInlineTextBox(curr)->getEmphasisMarkPosition(style, emphasisMarkPosition) && emphasisMarkPosition == TextEmphasisPositionOver) {

Do we know why style is null?  We should try to figure that out and add the explanation to the changelog.

> LayoutTests/fast/text/firstline/crash-firstline-detach-reload.html:34
> +        location.reload();

We don't want the test to be non-deterministic.  You might be able to further reduce the test case, which might help to understand the crash.  I'd be surprised if everything in the test case is necessary (an embed, a bdo, a different writing mode, a cursor and old flexbox?)

(In reply to comment #5)
> (From update of attachment 145948 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145948&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:1446
> > -            if (style->textEmphasisMark() != TextEmphasisMarkNone && toInlineTextBox(curr)->getEmphasisMarkPosition(style, emphasisMarkPosition) && emphasisMarkPosition == TextEmphasisPositionOver) {
> > +            if (style && style->textEmphasisMark() != TextEmphasisMarkNone && toInlineTextBox(curr)->getEmphasisMarkPosition(style, emphasisMarkPosition) && emphasisMarkPosition == TextEmphasisPositionOver) {
> 
> Do we know why style is null?  We should try to figure that out and add the explanation to the changelog.

The style appears to be null because the first-line style has been removed from the document, but a render object tagged as first-line still exists. I don't have enough knowledge of the rendering process to understand how this can happen, it appears to be some sort of race condition between rendering and reload.

A bisect pointed to http://trac.webkit.org/changeset/74326 -- attempting to reverse that change (which simply changes some layout bounds) also appears to avoid the specific problem in the repro.

> > LayoutTests/fast/text/firstline/crash-firstline-detach-reload.html:34
> > +        location.reload();
> 
> We don't want the test to be non-deterministic.  You might be able to further reduce the test case, which might help to understand the crash.  I'd be surprised if everything in the test case is necessary (an embed, a bdo, a different writing mode, a cursor and old flexbox?)

Agreed, but I have not been successful in reducing any further.

Created attachment 146414 [details]
Reduced testcase

Created attachment 146988 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146988 [details]
Patch

Attachment 146988 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12940544

New failing tests:
inspector/elements/edit-dom-actions.html

[WebKit Review Bot]

Created attachment 147021 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 147202 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147202 [details]
Patch

Attachment 147202 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12948699

New failing tests:
inspector/elements/edit-dom-actions.html

[WebKit Review Bot]

Created attachment 147248 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mike Lawther]

Hi Antti, Kling - this patch fixes a crash related to psuedo-style caching. The failing test you see is a timeout, which we're currently looking into.

In the meantime, does the approach to propagating cached pseudo-styles look OK? Would you recommend a different way?

Created attachment 147492 [details]
Patch

[Mike Lawther]

Hi Dan - smfr said you added pseudo-style caching a while back. Would you be able to take a look at this bug please (antti is currently on vacation)?

[Darin Adler]

Comment on attachment 147492 [details]
Patch

Concept of the patch looks fine.

How did you measure the performance impact of this additional work during style calculation?

[Darin Adler]

Dan is probably a better reviewer than I am for this, but despite that I am pretty sure this is good.