Bug 22590 – empty fragment href's crashes browser.

• Component: Page Loading
• Status: RESOLVED DUPLICATE

[Mahesh Kulkarni]

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.

[Mahesh Kulkarni]

Created attachment 25670 [details]
fix for empty fragment URL crash

assigning m_cssTarget to null if that node is getting deleted.

[Antti Koivisto]

Could you provide a test case as well?

[Mark Rowe (bdash)]

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

[Mahesh Kulkarni]

Created attachment 25712 [details]
test case

test case. click on first link, then click second and finally on third link!

[Darin Adler]

Comment on attachment 25670 [details]
fix for empty fragment URL crash

The fix looks fine. Now we need a regression test for this.

[Mahesh Kulkarni]

Please let me know what is the next thing I can do ?

[David Kilzer (:ddkilzer)]

(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:
http://webkit.org/coding/contributing.html

You must have a platform where layout tests are currently supported.  See this page for a list of ports that currently support the tests:
http://build.webkit.org/waterfall

[David Kilzer (:ddkilzer)]

(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?

[Mahesh Kulkarni]

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. 

[Darin Adler]

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.

[Darin Adler]

By adding a missing "preventDefault" to the test case I was able to reproduce a crash when MallocScribble is on, on Mac OS X.

[Darin Adler]

Created attachment 28415 [details]
test case with preventDefault added

[Darin Adler]

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.

[Adele Peterson]

Comment on attachment 25670 [details]
fix for empty fragment URL crash

R-'ing the patch until the test case is included as Darin described.

[Alexey Proskuryakov]

<rdar://problem/7325983>

[Alexey Proskuryakov]

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.

[Mahesh Kulkarni]

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 ***