Bug 22590 - empty fragment href's crashes browser.
Summary: empty fragment href's crashes browser.
Status: RESOLVED DUPLICATE of bug 20342
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Keywords: InRadar
: 22591 (view as bug list)
Depends on:
Reported: 2008-12-02 08:30 PST by Mahesh Kulkarni
Modified: 2011-03-08 11:57 PST (History)
4 users (show)

See Also:

fix for empty fragment URL crash (1019 bytes, patch)
2008-12-02 08:44 PST, Mahesh Kulkarni
adele: review-
Details | Formatted Diff | Diff
test case (347 bytes, text/html)
2008-12-03 09:29 PST, Mahesh Kulkarni
no flags Details
test case with preventDefault added (371 bytes, text/html)
2009-03-09 09:57 PDT, Darin Adler
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mahesh Kulkarni 2008-12-02 08:30:11 PST
On empty fragment url's, document::gotoanchor crashes browser. 
for ex:
<A href="#"> link </a>

Default behavior of empty fragment is to first check if cssTarget is specified, if not goto first element. In case if this cssTarget node is delete by Javascript, browser crashes clicking on "link" in the example.
Comment 1 Mahesh Kulkarni 2008-12-02 08:44:37 PST
Created attachment 25670 [details]
fix for empty fragment URL crash

assigning m_cssTarget to null if that node is getting deleted.
Comment 2 Antti Koivisto 2008-12-02 13:38:05 PST
Could you provide a test case as well?
Comment 3 Mark Rowe (bdash) 2008-12-02 16:02:09 PST
*** Bug 22591 has been marked as a duplicate of this bug. ***
Comment 4 Mahesh Kulkarni 2008-12-03 09:29:22 PST
Created attachment 25712 [details]
test case

test case. click on first link, then click second and finally on third link!
Comment 5 Darin Adler 2008-12-04 09:32:12 PST
Comment on attachment 25670 [details]
fix for empty fragment URL crash

The fix looks fine. Now we need a regression test for this.
Comment 6 Mahesh Kulkarni 2008-12-11 23:33:32 PST
Please let me know what is the next thing I can do ?
Comment 7 David Kilzer (:ddkilzer) 2008-12-12 04:47:15 PST
(In reply to comment #6)
> Please let me know what is the next thing I can do ?

See the "Regression Tests" section on this page:

You must have a platform where layout tests are currently supported.  See this page for a list of ports that currently support the tests:
Comment 8 David Kilzer (:ddkilzer) 2009-01-07 19:22:55 PST
(In reply to comment #4)
> Created an attachment (id=25712) [review]
> test case
> test case. click on first link, then click second and finally on third link! 

The test case doesn't crash for me on Safari 3.2 or with the most recent WebKit nightly build r39682.

Which build of WebKit do you see this crash in?
Comment 9 Mahesh Kulkarni 2009-01-22 04:15:12 PST
its a very much platform dependent crash. This case was crashing in nokia mobile browser, because there is a clear corrupt/deleted node* left behind. 
Comment 10 Darin Adler 2009-03-09 09:51:12 PDT
The reason the test case doesn't crash on Mac OS X WebKit is that navigating to a new URL results in a call to gotoAnchor in FrameLoader, and that function calls setCSSTarget(0) when you click on the second link. At that time, the object is still not deleted. I'll figure out why.
Comment 11 Darin Adler 2009-03-09 09:56:56 PDT
By adding a missing "preventDefault" to the test case I was able to reproduce a crash when MallocScribble is on, on Mac OS X.
Comment 12 Darin Adler 2009-03-09 09:57:28 PDT
Created attachment 28415 [details]
test case with preventDefault added
Comment 13 Darin Adler 2009-03-09 10:00:35 PDT
Someone needs to turn that test case into a regression test for the layout tests directory, and then make a patch that includes both the bug fix and the test case.

To make it so the test will show a crash without setting MallocScribble, at least in Debug builds, maybe we can add some code to the setChanged function in an assert. If that function called a virtual function, then I suspect we'd see a crash every time.
Comment 14 Adele Peterson 2009-03-23 11:15:10 PDT
Comment on attachment 25670 [details]
fix for empty fragment URL crash

R-'ing the patch until the test case is included as Darin described.
Comment 15 Alexey Proskuryakov 2009-10-22 00:11:33 PDT
Comment 16 Alexey Proskuryakov 2009-10-23 10:09:06 PDT
I cannot reproduce the crash with ToT. Actually, the code that sets CSS target to 0 was added in <http://trac.webkit.org/changeset/29311>, which was long before this bug was filed.

The code was moved around, and sometimes even removed (but it seems to have been present and functional when this bug was filed). Unfortunately, the original test that came with r29311 is currently disabled, which is tracked as bug 20342.
Comment 17 Mahesh Kulkarni 2011-03-08 11:57:31 PST
Marking this as fixed/duplicate of 20342 as it was fixed by Yael for qtWebkit.

*** This bug has been marked as a duplicate of bug 20342 ***