<iframe>s sometimes not painted after entering fullscreen in B2G

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug)

unspecified
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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.
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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.
Assignee: cpearce → nobody
blocking-basecamp: --- → ?
Component: DOM: Core & HTML → Layout
(Assignee)

Updated

5 years ago
Assignee: nobody → cpearce
(Assignee)

Updated

5 years ago
Summary: B2G Fullscreen in apps breaks when video present due to video controls → <iframe>s sometimes not painted after entering fullscreen in B2G
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 781014
(Assignee)

Updated

5 years ago
No longer blocks: 781014
blocking-basecamp: ? → +
(Assignee)

Comment 5

5 years ago
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
Attachment #651236 - Flags: review?(roc)
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #651236 - Attachment is obsolete: true
Attachment #651236 - Flags: review?(roc)
Attachment #651610 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 651611 [details] [diff] [review]
Patch 2 v2: Restore nsSubdocumentFrame presentation on reframe

With other review comments addressed.
Attachment #651611 - Flags: review?(roc)
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).
Attachment #651611 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 651617 [details] [diff] [review]
Patch 1 v2: Make nsRootPresContext::mRegisteredPlugins a refptr hash table

With review comments addressed.
Attachment #651610 - Attachment is obsolete: true
Attachment #651610 - Flags: review?(roc)
Attachment #651617 - Flags: review?(roc)
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.
Attachment #651617 - Flags: review?(roc) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c75f0428f8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcac58cbf328
https://hg.mozilla.org/mozilla-central/rev/9c75f0428f8a
https://hg.mozilla.org/mozilla-central/rev/bcac58cbf328
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 15

5 years ago
This patch most likely causes https://bugzilla.mozilla.org/show_bug.cgi?id=782979

Updated

5 years ago
Depends on: 782981
Backed out for causing bug 782981 on the latest Nightlies:
https://hg.mozilla.org/mozilla-central/rev/7d4268f8884c

Have triggered a Nightly respin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
The backout of this made bug 651866 go permaorange.
More likely to be the image stuff Kyle landed than a backout of this I would think. It was a backout afterall.
(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.
(Assignee)

Comment 20

5 years ago
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
Attachment #652650 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #652650 - Flags: review? → review?(roc)
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
Attachment #652650 - Flags: review?(roc) → review+
(Assignee)

Comment 22

5 years ago
Once more into the breach:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42af0a2ee337
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cfcc4e860c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2c8195793c
https://hg.mozilla.org/mozilla-central/rev/42af0a2ee337
https://hg.mozilla.org/mozilla-central/rev/0cfcc4e860c5
https://hg.mozilla.org/mozilla-central/rev/6f2c8195793c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 783824

Updated

5 years ago
Depends on: 781265
Depends on: 781279
No longer depends on: 781265
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?
(Assignee)

Updated

5 years ago
Depends on: 784837
(Assignee)

Comment 25

5 years ago



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

Updated

5 years ago
Depends on: 785808

Updated

5 years ago
QA Contact: jsmith
Whiteboard: [qa+]
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?
(Assignee)

Comment 27

5 years ago
There's a whole raft of B2G video issues at the moment, so you're probably seeing one of them.
Depends on: 786111

Updated

5 years ago
Whiteboard: [qa+] → [qa verification blocked]

Updated

5 years ago
Whiteboard: [qa verification blocked]
Depends on: 788436

Updated

5 years ago
Depends on: 818276

Updated

4 years ago
Depends on: 833823

Updated

4 years ago
Depends on: 930250

Updated

3 years ago
Depends on: 997255

Updated

2 years ago
Depends on: 1154219
Depends on: 1261230

Updated

11 months ago
Depends on: 1265577
You need to log in before you can comment on or make changes to this bug.