Bug 60449 - Full-screen video disappears behind black screen with scrollbar, followed by crash at jerryseinfeld.com
Summary: Full-screen video disappears behind black screen with scrollbar, followed by ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL: http://www.jerryseinfeld.com/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-08 01:04 PDT by Jer Noble
Modified: 2013-03-28 16:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2011-05-08 01:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2011-05-08 10:06 PDT, Jer Noble
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-05-08 01:04:45 PDT
1. Navigate to <http://jerryseinfeld.com/>
2. Click one of the video titles
3. When the video is playing, click the full screen button in the bottom right

The full-screen video is obscured by a black screen with what looks like a scrollbar track on the right. Moving the mouse sometimes hides the black screen, exposing strange-looking video control HUD, but other times leads to a Web Process crash.
Comment 1 Jer Noble 2011-05-08 01:05:15 PDT
<rdar://problem/9403197>
Comment 2 Jer Noble 2011-05-08 01:30:44 PDT
Created attachment 92733 [details]
Patch
Comment 3 Darin Adler 2011-05-08 08:13:43 PDT
Comment on attachment 92733 [details]
Patch

Normally every non-anonymous renderer has a 1-to-1 relationship with the DOM element that is returned by node(). I’m not sure it’s OK to mark something non-anonymous if it does not have that 1-to-1 relationship with a DOM element. It may fix the behavior of this one code path, but cause problems on other code paths.

Someone more expert than me in the rendering subsystem could weigh in, or we could do a bit more research about the other call sites that check the anonymous flag.

Another possibility is to add code at the call site that coalesces anonymous children.
Comment 4 Jer Noble 2011-05-08 09:23:55 PDT
(In reply to comment #3)
> (From update of attachment 92733 [details])
> Normally every non-anonymous renderer has a 1-to-1 relationship with the DOM element that is returned by node(). I’m not sure it’s OK to mark something non-anonymous if it does not have that 1-to-1 relationship with a DOM element. It may fix the behavior of this one code path, but cause problems on other code paths.
> 
> Someone more expert than me in the rendering subsystem could weigh in, or we could do a bit more research about the other call sites that check the anonymous flag.
> 
> Another possibility is to add code at the call site that coalesces anonymous children.

The other option I was considering was changing the definition of "isAnonymousBlock()". List marker renders are already excluded there (for good reason: you wouldn't want your list markers getting coalesced with the nearest non-marker anonymous block).  

I don't really understand the "1-to-1" comment. I though anonymous renderers were ones whose node() function returned the document? (From the RenderObject constructor initialization of m_isAnonymous.)
Comment 5 Darin Adler 2011-05-08 09:37:13 PDT
(In reply to comment #4)
> I thought anonymous renderers were ones whose node() function returned the document? (From the RenderObject constructor initialization of m_isAnonymous.)

That’s right. Generally speaking each element has exactly one non-anonymous renderer.
Comment 6 Jer Noble 2011-05-08 10:06:46 PDT
Created attachment 92737 [details]
Patch

This is the other option: excluding RenderFullScreen objects from isAnonymousBlock()
Comment 7 WebKit Review Bot 2011-05-08 10:09:29 PDT
Attachment 92737 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1

Source/WebCore/rendering/RenderObject.h:406:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Jer Noble 2011-05-08 10:23:18 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I thought anonymous renderers were ones whose node() function returned the document? (From the RenderObject constructor initialization of m_isAnonymous.)
> 
> That’s right. Generally speaking each element has exactly one non-anonymous renderer.

Oh right. "Normally every non-anonymous renderer has a 1-to-1 relationship..."  I somehow converted "non-anonymous" to "anonymous" somewhere between my eyes and brain. :)

Okay, so it appears the requirements for being an anonymous block are much more strict, and already set up to exclude certain renderers that would otherwise be counted as anonymous blocks.  And the number of call sites is much more limited.
Comment 9 Eric Seidel 2011-05-08 12:49:06 PDT
Comment on attachment 92737 [details]
Patch

The rules about what is anonymous vs. not seem very confusing. We recently had a MathML crasher for similar reasons (that blocks which should have been anonymous were not).  Perhaps someone can explain the anonymous renderer rules in a public place?
Comment 10 Jer Noble 2011-05-09 17:26:04 PDT
Committed r86109: <http://trac.webkit.org/changeset/86109>
Comment 11 Ademar Reis 2011-05-10 13:21:53 PDT
Revision r86109 cherry-picked into qtwebkit-2.2 with commit 9b3c5d2 <http://gitorious.org/webkit/qtwebkit/commit/9b3c5d2>
Comment 12 Eric Seidel 2013-03-28 13:37:26 PDT
Adding jchaffraix and esprehn, who may be able to explain the rules of anonymous renderers (here or in a Wiki).