Bug 37311 – Crash exiting fullscreen with captions enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

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

[Jer Noble]

Created attachment 52957 [details]
Patch

[WebKit Commit Bot]

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

[Jer Noble]

Created attachment 52960 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 52960 [details]
Patch

Clearing flags on attachment: 52960

Committed r57343: <http://trac.webkit.org/changeset/57343>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

We have code to make the commit-queue auto-rollout when it lands patches which break things.  Its intentionally disabled for now.

[Eric Seidel (no email)]

Reverted r57343 for reason:

Broke Tiger compile.

Committed r57348: <http://trac.webkit.org/changeset/57348>

[Jer Noble]

Created attachment 52992 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 52992 [details]
Patch

I didn't set commit-queue+ but some other reviewer could after getting the answer from Jer.

[Jer Noble]

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

[Jer Noble]

Created attachment 53011 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 53011 [details]
Patch

Clearing flags on attachment: 53011

Committed r57403: <http://trac.webkit.org/changeset/57403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.