Bug 10837 - REGRESSION: Yahoo New Charts Crashes WebKit.
Summary: REGRESSION: Yahoo New Charts Crashes WebKit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://finance.yahoo.com/charts#symbo...
Keywords: HasReduction, Regression
: 10955 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-09-13 06:30 PDT by David Scheck
Modified: 2006-10-07 05:43 PDT (History)
4 users (show)

See Also:


Attachments
Crash log (23.60 KB, text/plain)
2006-09-13 06:36 PDT, Mark Rowe (bdash)
no flags Details
Reduction (will assert or crash) (313 bytes, text/html)
2006-09-13 09:05 PDT, mitz
no flags Details
Proposed patch (11.25 KB, patch)
2006-09-20 23:11 PDT, Andrew Wellington
andrew: review-
Details | Formatted Diff | Diff
Proposed patch without the tabs (11.29 KB, patch)
2006-09-20 23:26 PDT, Andrew Wellington
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.