Bug 30018 - WebGL crashes with recent CanvasArray change
Summary: WebGL crashes with recent CanvasArray change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Blocker
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-02 08:34 PDT by Kenneth Russell
Modified: 2009-12-15 14:43 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.16 KB, patch)
2009-10-02 08:41 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Replacement patch eliminating stray tab (1.17 KB, patch)
2009-10-02 08:43 PDT, Kenneth Russell
oliver: review-
Details | Formatted Diff | Diff
Patch including test case (7.53 KB, patch)
2009-10-02 09:25 PDT, Kenneth Russell
oliver: review-
Details | Formatted Diff | Diff
Replacement patch with layouttest expected results (9.42 KB, patch)
2009-10-02 10:44 PDT, Chris Marrin
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2009-10-02 08:34:11 PDT
https://bugs.webkit.org/show_bug.cgi?id=29906 introduced a change to the CanvasArray base class which tests a PassRefPtr after its contents have been transferred to a RefPtr, causing the NULL check to always fail and the base pointer of the CanvasArray to always be NULL. This results in crashes on most if not all WebGL content. Patch is attached.
Comment 1 Kenneth Russell 2009-10-02 08:41:11 PDT
Created attachment 40519 [details]
Patch
Comment 2 Kenneth Russell 2009-10-02 08:43:26 PDT
Created attachment 40520 [details]
Replacement patch eliminating stray tab
Comment 3 Oliver Hunt 2009-10-02 08:52:51 PDT
Comment on attachment 40520 [details]
Replacement patch eliminating stray tab

Can you add a testcase for this?
Comment 4 Kenneth Russell 2009-10-02 09:25:40 PDT
Created attachment 40526 [details]
Patch including test case

Added unit test. Verified that test crashes before fix and runs successfully after fix.
Comment 5 Oliver Hunt 2009-10-02 09:52:11 PDT
Comment on attachment 40526 [details]
Patch including test case


> +<html xmlns="http://www.w3.org/1999/xhtml">
we don't use namespaces -- tests should basically be html5 doctype if you want strict mode.  But in this case you're also using the xml namespace which is also incorrect due to the file extension not being .xhtml, and not having an xml doctype :D

And you need expected output

Cheers,
  Oliver
Comment 6 Dimitri Glazkov (Google) 2009-10-02 09:59:52 PDT
... and don't forget -- WebKit style in JS also. 4-space, func brace on new line, etc. :)
Comment 7 Chris Marrin 2009-10-02 10:44:04 PDT
Created attachment 40533 [details]
Replacement patch with layouttest expected results
Comment 8 Oliver Hunt 2009-10-02 10:45:56 PDT
Comment on attachment 40533 [details]
Replacement patch with layouttest expected results

remove the superfluous namespace decl from <html> in the testcase
Comment 9 Chris Marrin 2009-10-02 10:50:07 PDT
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/canvas/webgl/array-unit-tests-expected.txt
Adding         LayoutTests/fast/canvas/webgl/array-unit-tests.html
Sending        WebCore/ChangeLog
Sending        WebCore/html/canvas/CanvasArray.cpp
Transmitting file data .....
Committed revision 49027.