Bug 119716 - Fix null dereference in HTMLAnchorElement::sendPings when frame is not attached to a page
Summary: Fix null dereference in HTMLAnchorElement::sendPings when frame is not attach...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on:
Blocks:
 
Reported: 2013-08-12 20:09 PDT by Ryosuke Niwa
Modified: 2013-08-13 00:09 PDT (History)
8 users (show)

See Also:

Attachments
Fixes the bug (1.50 KB, patch)
2013-08-12 20:10 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Adds a test (2.05 KB, patch)
2013-08-12 23:32 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Adds a test with real -expected.txt (2.41 KB, patch)
2013-08-12 23:34 PDT, Ryosuke Niwa 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Ryosuke Niwa 2013-08-12 20:10:49 PDT 
Created attachment 208588 [details]
Fixes the bug
Comment 2 Alexey Proskuryakov 2013-08-12 20:55:52 PDT 
Comment on attachment 208588 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=208588&action=review

The Blink change is quite suspicious, and I'd like to understand it better. Do you know why the bug is hidden?

> Source/WebCore/ChangeLog:10
> +        No new tests since the test in the Blink change doesn't reproduce crash on WebKit.

Does it reproduce the crash in Blink?

The test does a ton of weird things, and I'm not sure how those result in a frameless document. Perhaps they perform a synchronous navigation?

 I suggest to look into making a new test. What if one creates a new frameless document (with document.implementation.createHTMLDocument or with a parser), adds an anchor element with a ping attribute, and calls click() on it?
Comment 3 Darin Adler 2013-08-12 22:23:57 PDT 
Comment on attachment 208588 [details]
Fixes the bug

I agree that we should make a new test, but I also think adding the null check is fine and I trust Ryosuke to make the test.
Comment 4 Ryosuke Niwa 2013-08-12 22:46:36 PDT 
Comment on attachment 208588 [details]
Fixes the bug

Let me try creating a test following ap's suggestion.
Comment 5 WebKit Commit Bot 2013-08-12 22:47:25 PDT 
Comment on attachment 208588 [details]
Fixes the bug

Clearing flags on attachment: 208588

Committed r153975: <http://trac.webkit.org/changeset/153975>
Comment 6 WebKit Commit Bot 2013-08-12 22:47:28 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2013-08-12 22:52:06 PDT 
Oops, the commit queue outraced you! Sorry!!!
Comment 8 Ryosuke Niwa 2013-08-12 22:54:46 PDT 
Huh, it seems like cq landed it anyway :(
Comment 9 Ryosuke Niwa 2013-08-12 22:55:28 PDT 
I DID come up with a test case so let me upload it here.
Comment 10 Ryosuke Niwa 2013-08-12 23:32:39 PDT 
Created attachment 208598 [details]
Adds a test
Comment 11 Ryosuke Niwa 2013-08-12 23:34:58 PDT 
Created attachment 208599 [details]
Adds a test with real -expected.txt
Comment 12 Ryosuke Niwa 2013-08-12 23:37:41 PDT 
Committed r153982: <http://trac.webkit.org/changeset/153982>
Comment 13 Radar WebKit Bug Importer 2013-08-12 23:37:51 PDT 
<rdar://problem/14721165>
Comment 14 Radar WebKit Bug Importer 2013-08-12 23:37:55 PDT 
<rdar://problem/14721167>
Comment 15 Ryosuke Niwa 2013-08-13 00:09:57 PDT 
Note that I've confirmed that the landed test case will cause a crash if we didn't have my patch.

The reason I have to detach the frame in href is that HTMLAnchorElement::click has a check for the nullity of document()->frame() at the beginning. So I had to fool this code and detach the frame inside urlSelected.