Bug 37311 - Crash exiting fullscreen with captions enabled
Summary: Crash exiting fullscreen with captions enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P1 Critical
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-04-09 00:35 PDT by Jer Noble
Modified: 2010-04-09 19:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2010-04-09 08:53 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2010-04-09 09:31 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.22 KB, patch)
2010-04-09 14:55 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.41 KB, patch)
2010-04-09 16:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2010-04-09 00:35:13 PDT
In nightly builds of WebKit, the <video> element supports both captions and fullscreen. Each works well enough by itself, but we get a crash when exiting fullscreen with captions enabled (below).

To reproduce:

+ Navigate to a movie with Closed Captions
+ Click the "Toggle Close Captions" button
+ Click the "Fullscreen" button
+ Once in fullscreen, click the "exit fullscreen" icon

2010-03-03 12:20:34.373 Safari[40964:a0f] *** -[QTMovieViewCALayerRendererView _qtMovieClosedCaptionDidChange:]: message sent to deallocated instance 0x11d8fa070
Comment 1 Jer Noble 2010-04-09 08:53:07 PDT
Created attachment 52957 [details]
Patch
Comment 2 WebKit Commit Bot 2010-04-09 09:25:02 PDT
Comment on attachment 52957 [details]
Patch

Rejecting patch 52957 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
  /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5DE6D18C0FCF231B002DE28C.sh
** BUILD FAILED **

The following build commands failed:
WebKit:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/i386/WebVideoFullscreenController.o /Users/eseidel/Projects/CommitQueue/WebKit/mac/WebView/WebVideoFullscreenController.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/1669224
Comment 3 Jer Noble 2010-04-09 09:31:57 PDT
Created attachment 52960 [details]
Patch
Comment 4 WebKit Commit Bot 2010-04-09 09:53:24 PDT
Comment on attachment 52960 [details]
Patch

Clearing flags on attachment: 52960

Committed r57343: <http://trac.webkit.org/changeset/57343>
Comment 5 WebKit Commit Bot 2010-04-09 09:53:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Eric Seidel (no email) 2010-04-09 11:28:36 PDT
I believe this broke Tiger:
/Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/WebKit/mac/WebView/WebVideoFullscreenController.mm: In function 'void -[WebVideoFullscreenController windowDidLoad](WebVideoFullscreenController*, objc_selector*)':
/Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/WebKit/mac/WebView/WebVideoFullscreenController.mm:89: error: 'QTMovieLayer' was not declared in this scope
/Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/WebKit/mac/WebView/WebVideoFullscreenController.mm:89: error: 'layer' was not declared in this scope
cc1objplus: warnings being treated as errors
Comment 7 Eric Seidel (no email) 2010-04-09 12:04:31 PDT
We have code to make the commit-queue auto-rollout when it lands patches which break things.  Its intentionally disabled for now.
Comment 8 Eric Seidel (no email) 2010-04-09 12:10:26 PDT
Reverted r57343 for reason:

Broke Tiger compile.

Committed r57348: <http://trac.webkit.org/changeset/57348>
Comment 9 Jer Noble 2010-04-09 14:55:56 PDT
Created attachment 52992 [details]
Patch
Comment 10 Darin Adler 2010-04-09 14:58:40 PDT
Comment on attachment 52992 [details]
Patch

> -SOFT_LINK_CLASS(QTKit, QTMovieView)
> +SOFT_LINK_CLASS(QTKit, QTMovieLayer)

Does this need to go inside the ifdef?

> +#ifdef BUILDING_ON_TIGER
> +    // WebVideoFullscreenController is not supported on Tiger:
> +    ASSERT_NOT_REACHED();

I think it would be even better to put the entire body of the function inside the #ifdef.
Comment 11 Darin Adler 2010-04-09 14:59:12 PDT
Comment on attachment 52992 [details]
Patch

I didn't set commit-queue+ but some other reviewer could after getting the answer from Jer.
Comment 12 Jer Noble 2010-04-09 15:24:04 PDT
> Does this need to go inside the ifdef?

No, that macro expands into a set of ObjC functions which obtain a Class pointer at runtime.   There will be no compile-time references to QTMovieLayer, except in string form.  (i.e. "QTMovieLayer")

I'll also include the entire body of the two functions in the #ifdef and resubmit the patch.
Comment 13 Jer Noble 2010-04-09 16:08:24 PDT
Created attachment 53011 [details]
Patch
Comment 14 WebKit Commit Bot 2010-04-09 19:59:34 PDT
Comment on attachment 53011 [details]
Patch

Clearing flags on attachment: 53011

Committed r57403: <http://trac.webkit.org/changeset/57403>
Comment 15 WebKit Commit Bot 2010-04-09 19:59:40 PDT
All reviewed patches have been landed.  Closing bug.