Bug 12768 – REGRESSION (r19595): Crash in WebCore::RenderLayer::scrollToOffset leaving macupdate.com via bookmark

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kevin M. Dean]

Go to macupdate and page loads fine. Go to any other web site after macupdate loads and Webkit crashes.

[Kevin M. Dean]

Created attachment 13159 [details]
Crash log

[Mark Rowe (bdash)]

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?

[Kevin M. Dean]

I'm using a bookmark to go to another site. It seems that typing url does not trigger the crash as you found.

[Mark Rowe (bdash)]

Thanks, that reproduces perfectly now.  This is in radar as <rdar://problem/4995250>.

(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).

[Antti Koivisto]

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

(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.

[Antti Koivisto]

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 on attachment 13161 [details]
check for null view

What about other arbitrary blur event handlers? Are they safe to dispatch at that point?

[Antti Koivisto]

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.)

[Adele Peterson]

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.

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.

[Dave Hyatt]

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.

(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)).

[Dave Hyatt]

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...

(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).

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.

[Adele Peterson]

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.

[Adele Peterson]

Created attachment 13190 [details]
patch

[Adele Peterson]

Comment on attachment 13190 [details]
patch

clearing review flag. i'll post a new patch

[Adele Peterson]

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.

[Adele Peterson]

Created attachment 13199 [details]
new patch

[David Kilzer (:ddkilzer)]

Comment on attachment 13190 [details]
patch

For Mitz!

[Adele Peterson]

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.

[David Kilzer (:ddkilzer)]

Bug 12791 may be a duplicate (or related).

[Adele Peterson]

*** Bug 12791 has been marked as a duplicate of this bug. ***

Comment on attachment 13199 [details]
new patch

i like this patch. r=me!

[Adele Peterson]

Committed revision 19662.

*** Bug 12855 has been marked as a duplicate of this bug. ***