Bug 19470 - Null pointer dereferences on a document with no documentElement
Summary: Null pointer dereferences on a document with no documentElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Normal
Assignee: Nobody
URL: http://danceoffwithyourpantsoff.googl...
Keywords: HasReduction, InRadar
: 20853 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-06-10 17:21 PDT by danceoffwithyourpantsoff
Modified: 2008-09-15 07:04 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (2.26 KB, patch)
2008-06-13 11:23 PDT, danceoffwithyourpantsoff
eric: review-
Details | Formatted Diff | Diff
Proposed patch (4.97 KB, patch)
2008-06-13 13:41 PDT, danceoffwithyourpantsoff
darin: review+
Details | Formatted Diff | Diff
Proposed patch (5.26 KB, patch)
2008-06-13 16:01 PDT, danceoffwithyourpantsoff
darin: review-
Details | Formatted Diff | Diff
Patch attempt number 300 (5.17 KB, patch)
2008-06-16 10:19 PDT, danceoffwithyourpantsoff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description danceoffwithyourpantsoff 2008-06-10 17:21:39 PDT
There are a few pieces of code that don't expect documentElement() to return NULL, and these will lead to calling methods on a null pointer.  I've found 4 cases, but there are probably more.
Comment 1 Alexey Proskuryakov 2008-06-10 21:23:47 PDT
Confirmed with r34486.
Comment 2 Mark Rowe (bdash) 2008-06-11 00:28:23 PDT
<rdar://problem/5999631>
Comment 3 danceoffwithyourpantsoff 2008-06-13 11:23:31 PDT
Created attachment 21683 [details]
Proposed patch

Patch fixes the 4 issues in a pretty straightforward way.  I'm not sure about throwing HIERARCHY_REQUEST_ERR in setBody, maybe there is something better to throw?

I can make a layout test if needed.
Comment 4 Eric Seidel (no email) 2008-06-13 11:47:41 PDT
Comment on attachment 21683 [details]
Proposed patch

This needs a ChangeLog, see:
http://webkit.org/coding/contributing.html

And a test case.  (Also, next time, mark the change as r=? if you'd like it to be seen in the review queue. :)
Comment 5 danceoffwithyourpantsoff 2008-06-13 13:41:37 PDT
Created attachment 21687 [details]
Proposed patch
Comment 6 Darin Adler 2008-06-13 14:06:44 PDT
Comment on attachment 21687 [details]
Proposed patch

Looks fine. There are various small problems such as:

+        * LayoutTests\fast\dom\documentElement-null-expected.txt: Added.

Slashes are backwards here. Paths are from the top level, but should be relative to the ChangeLog file. LayoutTests needs its own separate ChangeLog file, not entries in the WebCore ChangeLog file.

+            if (const Element *doc = static_cast<const Document *>(this)->documentElement())
+                return doc->lookupPrefix(namespaceURI);
+            return String();

We put the * next to the type name, although the old code didn't do that. The newly added braces aren't needed -- the if statement already scopes the variable declared inside it.

I'd also suggest changing documentElement-null.html to create a document and put a message in at the end indicating success.

But I think the patch is good enough as-is that I'm going to review+ it. The person landing it can probably fix a few things, or someone could do a new patch.
Comment 7 danceoffwithyourpantsoff 2008-06-13 16:01:53 PDT
Created attachment 21691 [details]
Proposed patch
Comment 8 Darin Adler 2008-06-14 01:02:37 PDT
Comment on attachment 21691 [details]
Proposed patch

+  if (window.LayoutTestController)
+    window.LayoutTestController.dumpAsText();

The letter "L" has to be lowercase here, or it won't work.

Added: svn:eol-style
   + LF

Why LF style? We don't use that style for other files.

+            if (const Element* doc = static_cast<const Document*>(this)->documentElement())
+                return doc->isDefaultNamespace(namespaceURI);

We normally use "doc" for "document". Using "doc" for "documentElement" is unnecessarily confusing. Maybe "de" or "documentElement" or even just "e" or "element". Also really no need for const in the type of the local variable.

review- because of the letter "L" problem. I have no idea why the test worked!

Do you really want to go without a name in the change log? I'd prefer "Anonymous" to "danceoff" for the name here.
Comment 9 danceoffwithyourpantsoff 2008-06-16 10:19:42 PDT
Created attachment 21729 [details]
Patch attempt number 300
Comment 10 Darin Adler 2008-06-16 10:46:43 PDT
Comment on attachment 21729 [details]
Patch attempt number 300

r=me
Comment 11 danceoffwithyourpantsoff 2008-06-24 17:40:16 PDT
What happens now, will it get landed?
Comment 12 Alexey Proskuryakov 2008-06-24 22:20:45 PDT
This was landed in <http://trac.webkit.org/projects/webkit/changeset/34789>.
Comment 13 Alexey Proskuryakov 2008-09-15 07:04:21 PDT
*** Bug 20853 has been marked as a duplicate of this bug. ***