Bug 58719

Summary:

Cancelling fullscreen doesn't properly reset internal state

Product:

WebKit

Reporter:

Andrew Scherkus <scherkus>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, darin, eric, jer.noble, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	eric: review+, commit-queue: commit-queue-

Andrew Scherkus

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

Attachments
Patch (7.42 KB, patch) 2011-04-15 18:26 PDT, Andrew Scherkus	eric: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Andrew Scherkus

Comment 1 2011-04-15 18:26:42 PDT

Created attachment 89899 [details] Patch

Adam Barth

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

Andrew Scherkus

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

Jer Noble

Comment 4 2011-04-17 13:39:42 PDT

Looks good to me.

WebKit Commit Bot

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

WebKit Commit Bot

Comment 6 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

WebKit Review Bot

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

WebKit Commit Bot

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

Note You need to log in before you can comment on or make changes to this bug.