Bug 10837

Summary:

REGRESSION: Yahoo New Charts Crashes WebKit.

Product:

WebKit

Reporter:

David Scheck <opendarwin>

Component:

WebCore JavaScript

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, mitz, mrowe, troyb

Priority:

Keywords:

HasReduction, Regression

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

URL:

http://finance.yahoo.com/charts#symbol=AAPL;range=1d

Attachments:

Description	Flags
Crash log	none
Reduction (will assert or crash)	none
Proposed patch	andrew: review-
Proposed patch without the tabs	eric: review+

Description David Scheck 2006-09-13 06:30:26 PDT

The URL works in Safari but not Webkit.  

WebKit-SVN-r16342.dmg

Comment 1 Mark Rowe (bdash) 2006-09-13 06:36:12 PDT

Confirmed with r16342.

Comment 2 Mark Rowe (bdash) 2006-09-13 06:36:35 PDT

Created attachment 10524 [details]
Crash log

Comment 3 mitz 2006-09-13 08:19:48 PDT

ASSERTION FAILED: p (WebCore/bindings/js/kjs_window.cpp:378 static KJS::JSValue* KJS::Window::retrieve(WebCore::Frame*))

Comment 4 mitz 2006-09-13 09:05:05 PDT

Created attachment 10530 [details]
Reduction (will assert or crash)

The crash happens when trying to access the location property of a document that is no longer associated with a frame. I think the document's URL can be a good fallback in this case.

Comment 5 mitz 2006-09-20 22:27:21 PDT

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

Comment 6 Andrew Wellington 2006-09-20 23:11:44 PDT

Created attachment 10677 [details]
Proposed patch

Patch to return null in line with Firefox

Comment 7 Andrew Wellington 2006-09-20 23:25:28 PDT

Comment on attachment 10677 [details]
Proposed patch

Oi! Who put those tabs in there... :-/

Comment 8 Andrew Wellington 2006-09-20 23:26:05 PDT

Created attachment 10678 [details]
Proposed patch without the tabs

Comment 9 Eric Seidel (no email) 2006-09-21 02:31:05 PDT

Comment on attachment 10678 [details]
Proposed patch without the tabs

Looks good to me.  Too bad this logic can't be put in the DOM c++ implementation class.  That would sure make later auto-generation of JSHTMLDocument easier.

Curious.  I wonder if this is broken in the Obj-C version of this wrapper?

Otherwise it looks fine.  r=me.

Comment 10 Alexey Proskuryakov 2006-09-21 02:57:46 PDT

> Too bad this logic can't be put in the DOM c++ implementation class.

Cannot the check be inside retrieveWindow() itself?

Comment 11 Darin Adler 2006-10-06 07:35:28 PDT

(In reply to comment #10)
> > Too bad this logic can't be put in the DOM c++ implementation class.
> 
> Cannot the check be inside retrieveWindow() itself?

Yes. retrieveWindow should return 0 in this case. And we can change the jsUndefined() to jsNull().

We should make that change instead of this one.

Comment 12 Darin Adler 2006-10-06 07:37:14 PDT

(In reply to comment #9)
> (From update of attachment 10678 [details] [edit])
> Too bad this logic can't be put in the DOM C++ implementation class.

This logic *can* be put into the DOM C++ implementation class. I'm going to do that.

Comment 13 Darin Adler 2006-10-06 22:05:30 PDT

(In reply to comment #12)
> (In reply to comment #9)
> > (From update of attachment 10678 [details] [edit] [edit])
> > Too bad this logic can't be put in the DOM C++ implementation class.
> 
> This logic *can* be put into the DOM C++ implementation class. I'm going to do
> that.

Actually, it can't because there is no C++ implementation for this method yet. So the patch is fine as-is. Also, I think the frame nil check is a good solution -- no need to change Window::retrieve just to fix this, although we could consider making that change later.

Comment 14 Mark Rowe (bdash) 2006-10-07 05:43:50 PDT

Landed in r16881.