Bug 58719 - Cancelling fullscreen doesn't properly reset internal state
Summary: Cancelling fullscreen doesn't properly reset internal state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-15 18:21 PDT by Andrew Scherkus
Modified: 2011-06-18 12:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.42 KB, patch)
2011-04-15 18:26 PDT, Andrew Scherkus
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2011-04-15 18:21:04 PDT
Root problem is m_fullscreenElement remains non-null, which means webkitIsFullScreen() returns false but webkitCurrentFullScreenElement() returns the old (and I think detached!) element and a successive call to webkitCancelFullScreen() results in all sorts of bad stuff.
Comment 1 Andrew Scherkus 2011-04-15 18:26:42 PDT
Created attachment 89899 [details]
Patch
Comment 2 Adam Barth 2011-04-17 01:35:26 PDT
Comment on attachment 89899 [details]
Patch

I'm impressed that this patch is almost all minus lines, but I don't understand how this code works well enough to review your patch.
Comment 3 Andrew Scherkus 2011-04-17 11:31:18 PDT
Sorry for combining clean-up with a bug fix :(

In essence the fix is a one-liner: the addition of "m_fullScreenElement = 0" to webkitDidExitFullScreen

Technically m_isFullScreen should have guarded against being able to re-cancel full screen (see Document::webkitCancelFullScreen), but because we only check m_fullScreenElement, we execute the cancel code again and Bad Things happen.

Instead of having both m_isFullScreen and m_fullScreenElement track whether Document is in full screen, it seems safer to rely on whether m_fullScreenElement set.
Comment 4 Jer Noble 2011-04-17 13:39:42 PDT
Looks good to me.
Comment 5 WebKit Commit Bot 2011-04-18 12:14:50 PDT
The commit-queue encountered the following flaky tests while processing attachment 89899 [details]:

animations/suspend-resume-animation.html bug 48161 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2011-04-18 12:28:57 PDT
Comment on attachment 89899 [details]
Patch

Rejecting attachment 89899 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 2

Last 500 characters of output:
lay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating OpenSource
From git://git.webkit.org/WebKit
   e625813..122c5ca  master     -> origin/master
	M	LayoutTests/platform/qt/Skipped
	M	LayoutTests/ChangeLog
	M	Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
	M	Source/WebKit/qt/ChangeLog
r84168 = 122c5ca6e727a26fcffb5ef048533f56183e5a2e (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8469061
Comment 7 WebKit Review Bot 2011-04-18 13:26:24 PDT
http://trac.webkit.org/changeset/84169 might have broken Qt Linux Release
The following tests are not passing:
fast/ruby/after-block-doesnt-crash.html
fast/ruby/after-table-doesnt-crash.html
fast/ruby/before-block-doesnt-crash.html
fast/ruby/before-table-doesnt-crash.html
Comment 8 WebKit Commit Bot 2011-04-18 14:26:36 PDT
The commit-queue encountered the following flaky tests while processing attachment 89899 [details]:

http/tests/xmlhttprequest/re-login-async.html bug 51763 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.