Bug 35063 - Particularly constructed WebFrames can try to access a null HistoryItem
Summary: Particularly constructed WebFrames can try to access a null HistoryItem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 34905 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-02-17 16:41 PST by Brady Eidson
Modified: 2010-02-22 09:07 PST (History)
4 users (show)

See Also:


Attachments
Proposed fix, layout test, and Mac-only DRT changes. (15.25 KB, patch)
2010-02-17 16:58 PST, Brady Eidson
sam: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Javascript test case (438 bytes, text/html)
2010-02-20 20:23 PST, David Benjamin
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2010-02-17 16:41:27 PST
Starting with the changes for the history state object API, we do work to keep history items chained together after navigations.

There are some cases where this can occur when there wasn't an "m_currentItem" before a navigation took place.

All known cases seem to be using the Mac or (Apple)Windows API's [WebFrame loadData:] or [WebView initWithCoder:] which populate a WebFrame upon creation with content but never actually navigating it.

A subsequent intra-document navigation that attempts to chain together history items will result in a null deref.

I initially tried to fix this by *ALWAYS* giving HistoryController an empty m_currentItem at creation.  This resulted in many failing layout tests as the null-ness of the current item seems to be a linchpin when deciding whether a frame contains the "initial empty document" or has actually been navigated somewhere, and changing this throws off the back forward list in many tests in many different ways.

Cleaning that up is part of a much greater FrameLoader refactoring task.  For now, I'll be adding a proposed fix that simply does a null-check and early return in the known fragment identifier scrolling case.  Note that there are a few other "m_currentItem" null checks in HistoryController and - while FrameLoader should probably be reworked so they aren't necessary - it seems like this one was one that has simply stayed hidden until we started to see some crashes with particular API usage.

The patch will also continue - for now Mac-only - DRT changes that twiddle this crash with the very specific WebKit API usage needed.

In radar as <rdar://problem/7638892>
Comment 1 Brady Eidson 2010-02-17 16:58:03 PST
Created attachment 48947 [details]
Proposed fix, layout test, and Mac-only DRT changes.
Comment 2 WebKit Review Bot 2010-02-17 17:32:50 PST
Attachment 48947 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/279146
Comment 3 Brady Eidson 2010-02-17 19:25:37 PST
I don't understand how the Qt drt build wasn't already broken.  None of the LayoutTestController methods that take JSStringRefs seem to be implemented for Qt, how are they linking at all.

A review on the patch would still be appreciated while I try to figure out how to keep qt building...
Comment 4 Adam Roben (:aroben) 2010-02-18 08:20:59 PST
(In reply to comment #3)
> I don't understand how the Qt drt build wasn't already broken.  None of the
> LayoutTestController methods that take JSStringRefs seem to be implemented for
> Qt, how are they linking at all.
> 
> A review on the patch would still be appreciated while I try to figure out how
> to keep qt building...

Qt has its own implementation of LayoutTestController. It doesn't use the cross-platform one the rest of the ports use.
Comment 5 Brady Eidson 2010-02-18 09:13:59 PST
Landed in http://trac.webkit.org/changeset/54966
Comment 6 Tony Chang 2010-02-18 18:18:01 PST
*** Bug 34905 has been marked as a duplicate of this bug. ***
Comment 7 Darin Fisher (:fishd, Google) 2010-02-18 21:12:29 PST
FYI, it looks like Tony attached a test case to bug 34905 for this crash that just uses an iframe.
Comment 8 Brady Eidson 2010-02-19 09:16:31 PST
I saw that last night, and hope to adapt it to a new layout test when I have "spare cycles"
Comment 9 David Benjamin 2010-02-20 20:23:26 PST
Created attachment 49140 [details]
Javascript test case

> All known cases seem to be using the Mac or (Apple)Windows API's [WebFrame
> loadData:] or [WebView initWithCoder:] which populate a WebFrame upon creation
> with content but never actually navigating it.

I was looking into a separate crash recently and noticed that this also fixes it. You don't need to use WebKit APIs to trigger this; another case is if you open an empty window and then document.write() into it. I've attached the file I had been testing with.

(It appears that WebKit also doesn't follow what the standard says with regard to inserting history entries on document.open(). See 3.5.1. It looks like currently the type and replace arguments are ignored and no history modifications take place? Could be wrong about that --- only just started navigating the codebase.)
Comment 10 Brady Eidson 2010-02-22 09:07:20 PST
(In reply to comment #9)
> I was looking into a separate crash recently and noticed that this also fixes
> it. You don't need to use WebKit APIs to trigger this; another case is if you
> open an empty window and then document.write() into it. I've attached the file
> I had been testing with.

Thanks David, see the comments right above yours - this alternate way to crash was discovered after the fix and API test for this bug landed.

> (It appears that WebKit also doesn't follow what the standard says with regard
> to inserting history entries on document.open(). See 3.5.1. It looks like
> currently the type and replace arguments are ignored and no history
> modifications take place? Could be wrong about that --- only just started
> navigating the codebase.)

That seems completely orthogonal to this bug.  If you think something's wrong, please file a bugzilla with:
-A test case
-What you think expected behavior should be (and how it differs from real behavior)
-What other browsers do.