Bug 12768 - REGRESSION (r19595): Crash in WebCore::RenderLayer::scrollToOffset leaving macupdate.com via bookmark
Summary: REGRESSION (r19595): Crash in WebCore::RenderLayer::scrollToOffset leaving ma...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Adele Peterson
URL: http://www.macupdate.com/
Keywords: InRadar, Regression
: 12791 12855 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-02-13 16:33 PST by Kevin M. Dean
Modified: 2007-02-22 06:14 PST (History)
8 users (show)

See Also:


Attachments
Crash log (30.96 KB, text/plain)
2007-02-13 16:34 PST, Kevin M. Dean
no flags Details
check for null view (1.88 KB, patch)
2007-02-14 03:32 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Unsafe blur handler demo (367 bytes, text/html)
2007-02-14 14:23 PST, mitz
no flags Details
patch (6.83 KB, patch)
2007-02-15 14:32 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
new patch (11.43 KB, patch)
2007-02-16 00:24 PST, Adele Peterson
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin M. Dean 2007-02-13 16:33:46 PST
Go to macupdate and page loads fine. Go to any other web site after macupdate loads and Webkit crashes.
Comment 1 Kevin M. Dean 2007-02-13 16:34:19 PST
Created attachment 13159 [details]
Crash log
Comment 2 Mark Rowe (bdash) 2007-02-13 17:07:22 PST
I can't reproduce this.  I visited macupdate.com then typed a new URL in the address bar and let that new page load.  No crash.  Can you please provide more specific steps to reproduce the crash?
Comment 3 Kevin M. Dean 2007-02-13 17:25:57 PST
I'm using a bookmark to go to another site. It seems that typing url does not trigger the crash as you found.
Comment 4 Mark Rowe (bdash) 2007-02-13 17:32:35 PST
Thanks, that reproduces perfectly now.  This is in radar as <rdar://problem/4995250>.
Comment 5 mitz 2007-02-14 00:11:25 PST
(In reply to comment #3)
> I'm using a bookmark to go to another site. It seems that typing url does not
> trigger the crash as you found.
> 

The search field on the page should be focused at the time the new page is transitioned to. Focusing the URL field blurs the search field and avoids the crash.

The document's focused node is sent a blur event as the new page comes in. In trying to create a test case I noticed that the blur event is sent only if the document is kept in the page cache. I don't know if the event is supposed to be dispatched to a document that's being navigated away from, but I find it suspicious that it depends on whether the document is staying in the page cache or not (the reason being that Document::detach() nulls out the focused node only if the document is not in the page cache).
Comment 6 Antti Koivisto 2007-02-14 03:10:05 PST
Actually any site will now crash if you leave it with text field focused:

1) go to google
2) select a bookmark or drag-drop an url to the window

-> crash
Comment 7 mitz 2007-02-14 03:28:08 PST
(In reply to comment #6)
> Actually any site will now crash if you leave it with text field focused:

Any site that can enter the back/forward cache.

The regression is from <http://trac.webkit.org/projects/webkit/changeset/19595>, which made the blur events fire later than they used to.
Comment 8 Antti Koivisto 2007-02-14 03:32:11 PST
Created attachment 13161 [details]
check for null view

I don't think there is anything deep here, view can be null during document destruction and should be checked.
Comment 9 mitz 2007-02-14 03:40:28 PST
Comment on attachment 13161 [details]
check for null view

What about other arbitrary blur event handlers? Are they safe to dispatch at that point?
Comment 10 Antti Koivisto 2007-02-14 04:20:37 PST
Comment on attachment 13161 [details]
check for null view

You are right, it is pretty weird that  the event fires are that point. (though the incomplete view==0 check in scrollToOffset looks wrong too.)
Comment 11 Adele Peterson 2007-02-14 13:58:11 PST
We have other nil checks for the RenderView elsewhere in that function- so it makes sense to check there too.

Maybe we should make the nil check change and fix the event dispatch timing in a separate checkin.

Also, I tried making a layout test for this, but I think since it involves the B/F cache, I had trouble getting it to work.
Comment 12 mitz 2007-02-14 14:23:38 PST
Created attachment 13176 [details]
Unsafe blur handler demo

This is an example of a custom onblur handler that's unsafe to call after the document is detached. It crashes TOT when you click the button.
Comment 13 Dave Hyatt 2007-02-14 14:30:01 PST
Is willRemove being called on a document that is being purged from the b/f cache? Is that what's happening?  I'd really like to understand why willRemove is unsafe but detach was safe, since usually willRemove is called right before detach.
Comment 14 mitz 2007-02-14 14:52:23 PST
(In reply to comment #13)
> Is willRemove being called on a document that is being purged from the b/f
> cache? Is that what's happening?  I'd really like to understand why willRemove
> is unsafe but detach was safe, since usually willRemove is called right before
> detach.
> 

It's called under FrameLoader::clear(bool) and on a document leaving the frame (regardless of whether it's headed to the b/f cache or not. The reason the crash doesn't happen with pages that don't go into the b/f cache is that their detach() resets the focused node (and doesn't even dispatch blur events)).
Comment 15 Dave Hyatt 2007-02-14 15:26:37 PST
Is the issue that the tree is sitll in good enough shape to dispatch blurs during willRemove, but during detach, we detect we can't fire blur because we're doing teardown?

If so, we could in theory pass enough info to know that the document is being torn down so that willRemove could avoid firing the blur...
Comment 16 mitz 2007-02-14 15:50:10 PST
(In reply to comment #15)
> Is the issue that the tree is sitll in good enough shape to dispatch blurs
> during willRemove, but during detach, we detect we can't fire blur because
> we're doing teardown?

Documents going into the b/f cache aren't really torn down, detach() doesn't traverse the tree, so the blur-on-detach didn't use to happen in that case. For documents not going into the b/f cache, the focused node is reset before the any child nodes are detached, so again the blur event isn't dispatched.

> If so, we could in theory pass enough info to know that the document is being
> torn down so that willRemove could avoid firing the blur...

I think the info might be available e.g. in the form of document()->attached() being false (despite the node itself being attached).
Comment 17 mitz 2007-02-14 15:53:31 PST
By the way, in nightlies just before r19595, if you go back to a page with a focused text field, the field has focus but doesn't have a caret.
Comment 18 Adele Peterson 2007-02-15 13:31:38 PST
document()->attached() is still true when willRemove is called for this.  We'll have to find another way to detect that the document is being torn down.
Comment 19 Adele Peterson 2007-02-15 14:32:57 PST
Created attachment 13190 [details]
patch
Comment 20 Adele Peterson 2007-02-15 16:31:02 PST
Comment on attachment 13190 [details]
patch

clearing review flag. i'll post a new patch
Comment 21 Adele Peterson 2007-02-15 23:55:16 PST
The problem I'm considering now is how to handle the focus state in this case.   With this change, when you leave a page that's going in the cache, you don't clear out the focused node.  So when you restore that page from the cache, the focus will remain, but there will be no selection.  So for text controls, we'll have the "incomplete focus" problem again.

Here are a few solutions I'm considering:

1) Fix Document::setFocusedNode so you can clear out the focusedNode, without dispatching any events.

2) Leave event dispatch as is, nil check for the RenderView where appropriate.  Seems dangerous.

3) Don't clear out the focusedNode for documents that are in the page cache (which is what the previous patch does).  Call updateFocusAppearance for the focusedNode when you restore the PageState.  I haven't been able to get this to work for text fields in subframes.  But maybe that isn't important.

I'm leaning towards #3.
Comment 22 Adele Peterson 2007-02-16 00:24:15 PST
Created attachment 13199 [details]
new patch
Comment 23 David Kilzer (:ddkilzer) 2007-02-16 11:31:53 PST
Comment on attachment 13190 [details]
patch

For Mitz!
Comment 24 Adele Peterson 2007-02-16 11:37:08 PST
Comment on attachment 13199 [details]
new patch

OK.  I think this is ready for review.  I realized that we don't cache pages with frames in the b/f cache, so I added a fixme in PageState::restore that we may need to tweak that code when we add support for frames.
Comment 25 David Kilzer (:ddkilzer) 2007-02-16 11:41:57 PST
Bug 12791 may be a duplicate (or related).

Comment 26 Adele Peterson 2007-02-16 11:45:13 PST
*** Bug 12791 has been marked as a duplicate of this bug. ***
Comment 27 mitz 2007-02-16 11:47:52 PST
Comment on attachment 13199 [details]
new patch

i like this patch. r=me!
Comment 28 Adele Peterson 2007-02-16 11:51:49 PST
Committed revision 19662.
Comment 29 mitz 2007-02-22 06:14:20 PST
*** Bug 12855 has been marked as a duplicate of this bug. ***