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().
Created attachment 200816 [details] Patch
It looks like this entire feature came out of chromium a little while ago. Is anybody currently maintaining text autosizing?
Comment on attachment 200816 [details] Patch I’m not sure about this null check, Frame::Document() will only return null during teardown.
(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.
Created attachment 200819 [details] Patch
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?
(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?
(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.
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
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.
(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
Comment on attachment 200819 [details] Patch Get it out of the review queue, Mike will do a new patch soon anyway.
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.
(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.