Bug 27137 - REGRESSION (r44311): Reproducible crash due to infinite recursion into FrameLoader::gotoAnchor() -> FrameView::layout()
Summary: REGRESSION (r44311): Reproducible crash due to infinite recursion into FrameL...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Nobody
URL: http://hsivonen.iki.fi/doctype/#handling
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2009-07-09 16:43 PDT by mitz
Modified: 2009-07-10 09:57 PDT (History)
2 users (show)

See Also:


Attachments
Make lock-scroll-to-anchor a function of FrameView, and perform it as a post-layout task (11.27 KB, patch)
2009-07-09 23:04 PDT, mitz
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2009-07-09 16:43:49 PDT
<rdar://problem/7043124>

VIsiting the URL makes Safari crash with a stack showing many repititions of

  com.apple.WebCore              0x9547f226 WebCore::FrameLoader::gotoAnchor() + 0x56
  com.apple.WebCore              0x9549a348 WebCore::FrameView::layout(bool) + 0x828
  com.apple.WebCore              0x956124ad WebCore::FrameLoader::gotoAnchor(WebCore::String const&) + 0x26d

We may need to make the going-to-anchor a post-layoiut task. I suspect that this was introduced by a recent patch to improve locking-to-anchor during loading, but I haven’t verified this yet.
Comment 1 mitz 2009-07-09 16:47:48 PDT
<http://trac.webkit.org/changeset/44311/> is the change I had in mind.
Comment 2 mitz 2009-07-09 16:56:53 PDT
Confirmed that reverting r44311 fixes the crash.
Comment 3 mitz 2009-07-09 17:20:19 PDT
Having looked at the code, I don’t think moving to post-layout is the right solution here. If the frame needs layout at that point, there’s no use scrolling it until that layout occurs. But also, it seems somewhat inefficient to go through gotoAnchor() every time. Perhaps instead of the lockedToAnchor boolean, a RefPtr<Node> could be used and the frame could track that node without calling back into the loader.
Comment 4 Adam Barth 2009-07-09 19:08:27 PDT
Concensus on IRC was not to back the patch out tonight.  Nate will look into this tomorrow.  If we can't find a fix shortly, we will revert the offending change.
Comment 5 mitz 2009-07-09 23:04:03 PDT
Created attachment 32551 [details]
Make lock-scroll-to-anchor a function of FrameView, and perform it as a post-layout task
Comment 6 Simon Fraser (smfr) 2009-07-10 08:13:16 PDT
Comment on attachment 32551 [details]
Make lock-scroll-to-anchor a function of FrameView, and perform it as a post-layout task



> +void FrameView::keepScrolledToAnchorNode(Node* anchorNode)

I'm not a big fan of this method name; maybe something like maintainScrollPositionToAnchorNode?

> +    if (anchorNode != m_frame->document())
> +        rect = anchorNode->getRect();

getRect(true) will make this transform-friendly!

r=me