Bug 19470 – Null pointer dereferences on a document with no documentElement

• Component: DOM
• Status: RESOLVED FIXED

[danceoffwithyourpantsoff]

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.

[Alexey Proskuryakov]

Confirmed with r34486.

[Mark Rowe (bdash)]

<rdar://problem/5999631>

[danceoffwithyourpantsoff]

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.

[Eric Seidel (no email)]

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

[danceoffwithyourpantsoff]

Created attachment 21687 [details]
Proposed patch

[Darin Adler]

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.

[danceoffwithyourpantsoff]

Created attachment 21691 [details]
Proposed patch

[Darin Adler]

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.

[danceoffwithyourpantsoff]

Created attachment 21729 [details]
Patch attempt number 300

[Darin Adler]

Comment on attachment 21729 [details]
Patch attempt number 300

r=me

[danceoffwithyourpantsoff]

What happens now, will it get landed?

[Alexey Proskuryakov]

This was landed in <http://trac.webkit.org/projects/webkit/changeset/34789>.

[Alexey Proskuryakov]

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