Last Comment Bug 775965 - <iframe>s sometimes not painted after entering fullscreen in B2G
: <iframe>s sometimes not painted after entering fullscreen in B2G
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Chris Pearce (:cpearce)
: Jason Smith [:jsmith]
Mentors:
Depends on: 997255 781279 782981 783824 784837 785808 CVE-2012-4181 788436 818276 833823 930250 1154219 1261230 1265577
Blocks: 684620
  Show dependency treegraph
 
Reported: 2012-07-20 07:38 PDT by Chris Pearce (:cpearce)
Modified: 2016-05-20 15:23 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Patch v1: Ensure subdocframe presentation persists across reframes (15.06 KB, patch)
2012-08-12 15:44 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v1: Make nsRootPresContext::mRegisteredPlugins a refptr hash table (9.00 KB, patch)
2012-08-13 18:48 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v2: Restore nsSubdocumentFrame presentation on reframe (15.07 KB, patch)
2012-08-13 18:50 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 1 v2: Make nsRootPresContext::mRegisteredPlugins a refptr hash table (11.79 KB, patch)
2012-08-13 19:46 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 3 v1: store rootprescontext on nsObjectFrame when registering plugins for geometry updates (7.15 KB, patch)
2012-08-16 19:19 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-07-20 07:38:35 PDT
For some reason that I haven't figured out yet, B2G fullscreen in apps breaks in the presence of a <video> element in the document. When fullscreening a document in an app which contains a <video> element, we go fullscreen but we only display a blank white screen, rather than the document fullscreen'd. This is somehow caused by the video controls; if I disable the video controls binding, the problem goes away and fullscreen works as expected.
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-07-20 09:18:25 PDT
How are you disabling the video controls binding? By removing the |controls| attribute? (Just curious, it probably won't make a difference but if I get a chance to try to repro locally it will help to follow the same steps.)

There is one "mozfullscreenchange" listener in the video controls but the code that it runs seems innocuous.
Comment 2 Chris Pearce (:cpearce) 2012-07-20 09:51:34 PDT
I had to remove the bindings themselves in html.css, b2g/chrome/content/content.css and b2g/chrome/jar.nm.

The problem happens regardless of whether the controls attribute is set; we seem to always be initializing the controls (at least on B2G) on video elements.

Strangely fullscreen works in the video player app, just not my test app at http://pearce.org.nz/f
Comment 3 Chris Pearce (:cpearce) 2012-08-06 20:14:19 PDT
This isn't caused by the video controls.

If I have an app with an iframe which takes a while to load (say an <iframe src="http://cnn.com">) the same problem exists (i.e. when I go fullscreen the background of the iframe's containing document is rendered and the iframe's content is not rendered, though the border is).

On further investigation, it appears that we're failing in nsSubDocumentFrame::BuildDisplayList() in the !subdocView path, i.e. our inner view's first child is null, we exit here:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#249

Roc suggests that to fix this we should make changing an iframe to position:fixed not cause the subdocument to be reframed (we make containing frames position:fixed when we enter fullscreen). This was discussed in bug 708553 comment 22 and later.
Comment 4 Chris Pearce (:cpearce) 2012-08-06 20:22:44 PDT
(In reply to Chris Pearce (:cpearce) from comment #3)
> Roc suggests that to fix this we should make changing an iframe to
> position:fixed not cause the subdocument to be reframed[...]

I meant, we should not destroy/recreate the subdocument presentation when reconstructing nsSubdocumentFrame.
Comment 5 Chris Pearce (:cpearce) 2012-08-12 15:44:33 PDT
Created attachment 651236 [details] [diff] [review]
Patch v1: Ensure subdocframe presentation persists across reframes

Stash the root view and its siblings on the frame loader when nsSubdocumentFrame is destroyed, and restore it when the nsSubdocumentFrame is recreated.

We also detect if an iframe which is being reframed changes owner document, and we throw away the presentation if that's the case. Roc: Is this what you meant, or should we throw away the presentation if the *subdocument* changes?

Greenish on Try:
https://tbpl.mozilla.org/?tree=Try&rev=8b6c3389f176
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-12 17:32:02 PDT
Comment on attachment 651236 [details] [diff] [review]
Patch v1: Ensure subdocframe presentation persists across reframes

Review of attachment 651236 [details] [diff] [review]:
-----------------------------------------------------------------

I'm worried about the way this leaves dangling pointers in the nsRootPresContext::mRegisteredPlugins between the destruction and recreation of an nsSubDocumentFrame containing plugins. I think for safety we shouldn't do that.

I think the best fix for that would be to make nsRootPresContext::mRegisteredPlugins be a set of nsRefPtr<nsObjectLoadingContent>. Then PluginBoundsEnumerator would get the frame by calling GetPrimaryFrame and if the frame has been destroyed for whatever reason, that will return null and we can bail out safely. Similar in PluginHideEnumerator and PluginDidSetGeometryEnumerator.

::: content/base/src/nsFrameLoader.cpp
@@ +2393,5 @@
> +}
> +
> +void
> +nsFrameLoader::GetDetachedSubdocView(nsIView** aDetachedViews,
> +                                     nsIDocument** aSubdoc) const

One of these can be returned as a direct result.

::: content/base/src/nsFrameLoader.h
@@ +349,5 @@
> +  // Stores the document of the presentation that is stashed in
> +  // mDetachedSubdocViews. This allows us to detect whether the subdocument
> +  // has changed during a reframe, so that we know not to restore the
> +  // presentation.
> +  nsCOMPtr<nsIDocument> mDetachedSubdoc;

You should be checking whether the *container* document has changed, not the subdocument.

::: layout/generic/nsSubDocumentFrame.cpp
@@ +151,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>    EnsureInnerView();
>  
> +  // If we have a detach subdoc's root view on our frame loader, re-insert

detached

::: layout/generic/test/test_presentation_persist_across_reframe.html
@@ +20,5 @@
> +  <embed type="application/x-test" width="200" height="200"></embed>
> + </body>
> +</html>
> +-->
> +<iframe id="iframe1" src="data:text/html;charset=utf-8;base64,PGh0bWw%2BDQogPGJvZHk%2BDQogIDxlbWJlZCB0eXBlPSJhcHBsaWNhdGlvbi94LXRlc3QiIHdpZHRoPSIyMDAiIGhlaWdodD0iMjAwIj48L2VtYmVkPg0KIDwvYm9keT4NCjwvaHRtbD4NCg%3D%3D"></iframe>

Don't base-64 encode this. You can just embed the source (with single-quoted attributes).

@@ +36,5 @@
> +  var iframe2 = document.getElementById("iframe2");
> +  iframe1.parentNode.removeChild(iframe1);
> +  iframe1.style.position = "fixed";
> +  iframe2.contentDocument.body.appendChild(iframe1);
> +  setTimeout(function() { ok(true, "We didn't crash!"); SimpleTest.finish(); }, 0);

This test is poorly named. It claims to be testing that a presentation persists across reframe. But it only tests that we don't crash when moving one iframe into another. Fix the name.
Comment 7 Chris Pearce (:cpearce) 2012-08-13 18:48:47 PDT
Created attachment 651610 [details] [diff] [review]
Patch 1 v1: Make nsRootPresContext::mRegisteredPlugins a refptr hash table

I've split the nsRootPresContext:mRegisteredPlugins change into a separate patch, seeing as it changes different code.

I made mRegisteredPlugins a nsTHashtable<nsRefPtrHashKey<nsIContent> > rather than a nsTHashtable<nsRefPtrHashKey<nsObjectLoadingContent> > , since using an nsIContent that makes the type coercion/casting easier.
Comment 8 Chris Pearce (:cpearce) 2012-08-13 18:50:02 PDT
Created attachment 651611 [details] [diff] [review]
Patch 2 v2: Restore nsSubdocumentFrame presentation on reframe

With other review comments addressed.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-13 18:59:20 PDT
Comment on attachment 651610 [details] [diff] [review]
Patch 1 v1: Make nsRootPresContext::mRegisteredPlugins a refptr hash table

Review of attachment 651610 [details] [diff] [review]:
-----------------------------------------------------------------

You need to change PluginHideEnumerator. I don't know how this builds without that.

::: layout/base/nsPresContext.cpp
@@ +2500,3 @@
>  {
>    PluginGeometryClosure* closure = static_cast<PluginGeometryClosure*>(userArg);
> +  nsCOMPtr<nsIContent> content = aEntry->GetKey();

I think this can be declared nsIContent*. Adding/removing refcounts here is unneeded.
In fact we don't even need the local variable.

@@ +2500,4 @@
>  {
>    PluginGeometryClosure* closure = static_cast<PluginGeometryClosure*>(userArg);
> +  nsCOMPtr<nsIContent> content = aEntry->GetKey();
> +  nsObjectFrame* f = static_cast<nsObjectFrame*>(content->GetPrimaryFrame());

Bail out here if f is null (maybe with an NS_WARNING).

@@ +2785,2 @@
>  {
> +  nsObjectFrame* f = static_cast<nsObjectFrame*>(aEntry->GetKey()->GetPrimaryFrame());

Bail out here if f is null (maybe with an NS_WARNING).
Comment 10 Chris Pearce (:cpearce) 2012-08-13 19:10:33 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 651610 [details] [diff] [review]
> Patch 1 v1: Make nsRootPresContext::mRegisteredPlugins a refptr hash table
> 
> Review of attachment 651610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to change PluginHideEnumerator. I don't know how this builds without that.

PluginHideEnumerator only enumerates over PluginGeometryClosure::mAffectedPlugins, which is only live while nsRootPresContext::GetPluginGeometryUpdates() is on the stack. 

I suppose I could change it for consistency.
Comment 11 Chris Pearce (:cpearce) 2012-08-13 19:46:33 PDT
Created attachment 651617 [details] [diff] [review]
Patch 1 v2: Make nsRootPresContext::mRegisteredPlugins a refptr hash table

With review comments addressed.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-13 20:26:22 PDT
Comment on attachment 651617 [details] [diff] [review]
Patch 1 v2: Make nsRootPresContext::mRegisteredPlugins a refptr hash table

Review of attachment 651617 [details] [diff] [review]:
-----------------------------------------------------------------

You were right the first time. PluginHideEnumerator doesn't need to be modified here, and shouldn't be. r+ with that change reverted to your previous patch.
Comment 15 Octoploid 2012-08-15 09:11:23 PDT
This patch most likely causes https://bugzilla.mozilla.org/show_bug.cgi?id=782979
Comment 16 Ed Morley [:emorley] 2012-08-15 10:12:08 PDT
Backed out for causing bug 782981 on the latest Nightlies:
https://hg.mozilla.org/mozilla-central/rev/7d4268f8884c

Have triggered a Nightly respin.
Comment 17 Ed Morley [:emorley] 2012-08-15 13:04:10 PDT
The backout of this made bug 651866 go permaorange.
Comment 18 Timothy Nikkel (:tnikkel) 2012-08-15 14:19:34 PDT
More likely to be the image stuff Kyle landed than a backout of this I would think. It was a backout afterall.
Comment 19 Ed Morley [:emorley] 2012-08-16 07:57:33 PDT
(In reply to Timothy Nikkel (:tn) from comment #18)
> More likely to be the image stuff Kyle landed than a backout of this I would
> think. It was a backout afterall.

Sorry, you are right.

A couple of unluckily placed greens (bug 651866 isn't quite permaorange it would seem) implicated the wrong changeset. Bug 685516 is at fault instead.
Comment 20 Chris Pearce (:cpearce) 2012-08-16 19:19:25 PDT
Created attachment 652650 [details] [diff] [review]
Patch 3 v1: store rootprescontext on nsObjectFrame when registering plugins for geometry updates

This patch fixes the gmail/tab close crash by storing a reference to the root pres context which plugins are registered for geometry updates on the nsObjectFrame.

Looks like it's going green on try:
https://tbpl.mozilla.org/?tree=Try&rev=82143cfbe90c
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-16 20:14:40 PDT
Comment on attachment 652650 [details] [diff] [review]
Patch 3 v1: store rootprescontext on nsObjectFrame when registering plugins for geometry updates

Review of attachment 652650 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsObjectFrame.h
@@ +272,5 @@
> +  // We keep this reference to ensure we can always unregister the
> +  // plugins we register on the root PresContext.
> +  // This is only non-null while we have a plugin registered for geometry
> +  // updates.
> +  nsRefPtr<nsRootPresContext> mRootPresContext;

Call it mRootPresContextRegisteredWith
Comment 24 Jonathan Kew (:jfkthame) 2012-08-22 14:23:54 PDT
This has caused problems for the HiDPI support being developed in bug 674373, as described in comments 217, 220 and following. In certain cases the content of an iframe gets rendered at 50% of its proper size; it looks as if the presentation of the frame contents was initialized for a non-HiDPI context, then got moved into a HiDPI context without recomputing the device-pixel dimensions of its layers etc.

If I disable the use of the "stashed" presentation at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#166, this problem goes away - but that would effectively defeat the purpose of this bug. Can we add a condition here so that we don't use the stashed presentation if it was created for an incompatible context? E.g. if the viewManagers for detachedViews and mInnerView refer to different device contexts, then don't use the stashed presentation?
Comment 25 Chris Pearce (:cpearce) 2012-08-22 15:11:43 PDT



(In reply to Jonathan Kew (:jfkthame) from comment #24)
> Can we add a condition here so
> that we don't use the stashed presentation if it was created for an
> incompatible context? E.g. if the viewManagers for detachedViews and
> mInnerView refer to different device contexts, then don't use the stashed
> presentation?

Certainly. I filed Bug 784837 for fixing this. I don't have the equipment to test this, so I'd need your help testing this...
Comment 26 Jason Smith [:jsmith] 2012-08-29 11:27:21 PDT
Chris - I tried going to your test page here - http://pearce.org.nz/fullscreen/ for this bug. However, the video doesn't render at all. Separate issue or is this bug not fixed?
Comment 27 Chris Pearce (:cpearce) 2012-08-29 12:14:17 PDT
There's a whole raft of B2G video issues at the moment, so you're probably seeing one of them.

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