Bug 115677 – FrameView::setFrameRect can crash when ENABLE(TEXT_AUTOSIZING)

• Component: Text
• Status: NEW

[Mike Lattanzio]

The #if ENABLE(TEXT_AUTOSIZING) block of FrameView::setFrameRect looks incorrect.

It loops over all of the frames in the page, but it uses m_frame instead of the loop variable. 

Also, it's possible for frame->document() to return 0 on BlackBerry anyway and I suspect that's the case on other ports as well. 
We need to check frame->document() before calling frame->document()->textAutosizer()->recalculateMultipliers().

[Mike Lattanzio]

Created attachment 200816 [details]
Patch

[Mike Lattanzio]

It looks like this entire feature came out of chromium a little while ago. 

Is anybody currently maintaining text autosizing?

[Anders Carlsson]

Comment on attachment 200816 [details]
Patch

I’m not sure about this null check, Frame::Document() will only return null during teardown.

[Mike Lattanzio]

(In reply to comment #3)
> (From update of attachment 200816 [details])
> I’m not sure about this null check, Frame::Document() will only return null during teardown.

Okay it was definitely exploding on me, probably in teardown. I think I could just move the whole block later after we get and check the Document* document.

[Mike Lattanzio]

Created attachment 200819 [details]
Patch

[Andreas Kling]

Comment on attachment 200819 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests as there is no new functionality.

This is contradictory to the preceding lines.
There's a crash fix and a correctness fix in this patch, are they both untestable?

[Andreas Kling]

(In reply to comment #6)
> (From update of attachment 200819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200819&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests as there is no new functionality.
> 
> This is contradictory to the preceding lines.
> There's a crash fix and a correctness fix in this patch, are they both untestable?

Also, do you have a backtrace for the crash?

[Mike Lattanzio]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 200819 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=200819&action=review
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        No new tests as there is no new functionality.
> > 
> > This is contradictory to the preceding lines.
> > There's a crash fix and a correctness fix in this patch, are they both untestable?
> 
> Also, do you have a backtrace for the crash?

Yes for the BlackBerry port. 

I'll put a but more thought into what's testable and put together a more thorough patch tomorrow. Thanks for the quick feedback guys.

[Mike Lattanzio]

Crashing stack when navigating backward. BlackBerry port.

Program terminated with signal 11, Segmentation fault.
#0  WebCore::FrameView::setFrameRect (this=0x79560370, newRect=...)
    at /home/mlattanzio/dev/webkit/Source/WebCore/page/FrameView.cpp:464
464	                m_frame->document()->textAutosizer()->recalculateMultipliers();
(gdb) p m_frame
$1 = {m_ptr = 0x795fe8e8}
(gdb) p (m_frame.m_ptr->m_doc)
$2 = {m_ptr = 0x0}
(gdb) bt
#0  WebCore::FrameView::setFrameRect (this=0x79560370, newRect=...)
    at /home/mlattanzio/dev/webkit/Source/WebCore/page/FrameView.cpp:464
#1  0x7a48d74a in WebCore::FrameLoader::open (this=0x795fe928, cachedFrame=...)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:2043
#2  0x7ac29126 in WebCore::CachedFrame::open (this=0x799e6258)
    at /home/mlattanzio/dev/webkit/Source/WebCore/history/CachedFrame.cpp:213
#3  0x7ac297b8 in WebCore::CachedPage::restore (this=0x799e4b40, page=0x79523440)
    at /home/mlattanzio/dev/webkit/Source/WebCore/history/CachedPage.cpp:82
#4  0x7a4922b6 in WebCore::FrameLoader::commitProvisionalLoad (this=0x795fe928)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:1770
#5  0x7a492672 in WebCore::FrameLoader::loadProvisionalItemFromCachedPage (this=0x795fe928)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:3019
#6  0x7a492788 in WebCore::FrameLoader::continueLoadAfterNavigationPolicy (this=0x795fe928, formState=..., 
    shouldContinue=<optimized out>) at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:2882
#7  0x7a492818 in WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy (argument=<optimized out>, request=..., 
    formState=..., shouldContinue=<optimized out>) at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:2759
#8  0x7a4a5200 in WebCore::PolicyChecker::checkNavigationPolicy (this=0x795fe930, request=..., loader=0x799650d8, 
    formState=..., 
    function=0x7a492801 <WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>, argument=0x795fe928)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/PolicyChecker.cpp:71
#9  0x7a493136 in WebCore::FrameLoader::loadWithDocumentLoader (this=0x795fe928, loader=0x799650d8, type=<optimized out>, 
    prpFormState=...) at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:1435
#10 0x7a4958d2 in loadDifferentDocumentItem (
    cacheLoadPolicy=WebCore::FrameLoader::MayAttemptCacheOnlyLoadForFormSubmissionItem, 
    loadType=WebCore::FrameLoadTypeIndexedBackForward, item=0x795bfc38, this=0x795fe928)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:3117
#11 WebCore::FrameLoader::loadItem (this=0x795fe928, item=0x795bfc38, loadType=WebCore::FrameLoadTypeIndexedBackForward)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/FrameLoader.cpp:3210
#12 0x7a4981ee in WebCore::HistoryController::recursiveGoToItem (this=0x795fead0, item=0x795bfc38, fromItem=0x79536920, 
    type=WebCore::FrameLoadTypeIndexedBackForward)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/HistoryController.cpp:779
#13 0x7a49847c in WebCore::HistoryController::goToItem (this=0x795fead0, targetItem=0x795bfc38, 
    type=WebCore::FrameLoadTypeIndexedBackForward)
    at /home/mlattanzio/dev/webkit/Source/WebCore/loader/HistoryController.cpp:320
#14 0x7a50d654 in WebCore::Page::goToItem (this=0x79523440, item=0x795bfc38, type=WebCore::FrameLoadTypeIndexedBackForward)
    at /home/mlattanzio/dev/webkit/Source/WebCore/page/Page.cpp:418
#15 0x7a1a24c8 in BlackBerry::WebKit::WebPage::goBackOrForward (this=0x7941cc80, delta=-1)
    at /home/mlattanzio/dev/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3063
#16 0x78c87b4e in ?? () from libwebview.so.2
#17 0x78c87b4e in ?? () from libwebview.so.2

[Mike Lattanzio]

Based on that stack and my own reproduction steps it does crash when navigating backwards sometimes. I believe relocating the block that calls recalculateMultipliers below where we fetch the Document* will work.

I'm not sure what other ports build with ENABLE_TEXT_AUTOSIZING but now that's its been enabled on ours I see that we're crashing whenever we navigate backwards in history from just about any page to our landing page about:newtab.

I'm not sure how to construct a test that replicates this phenomenon, or if its even reproducible on any other port.

[Antonio Gomes]

(In reply to comment #10)
> Based on that stack and my own reproduction steps it does crash when navigating backwards sometimes. I believe relocating the block that calls recalculateMultipliers below where we fetch the Document* will work.
> 
> I'm not sure what other ports build with ENABLE_TEXT_AUTOSIZING but now that's its been enabled on ours I see that we're crashing whenever we navigate backwards in history from just about any page to our landing page about:newtab.
> 
> I'm not sure how to construct a test that replicates this phenomenon, or if its even reproducible on any other port.

- Test can go within LayoutTest/platform/blackberry
- you can enable page cache on DRT/Internals
- you can enable (should be able to) enable text autosizing

[Rob Buis]

Comment on attachment 200819 [details]
Patch

Get it out of the review queue, Mike will do a new patch soon anyway.

[Jacky Jiang]

Same crash case as this one http://trac.webkit.org/changeset/147949 which has been landed,  so I think fixing a crash here makes sense as well.

[Antonio Gomes]

(In reply to comment #13)
> Same crash case as this one http://trac.webkit.org/changeset/147949 which has been landed,  so I think fixing a crash here makes sense as well.

I agree. we just need a test.