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.

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

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.

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

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

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.

With other review comments addressed.

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

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

With review comments addressed.

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.

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

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

Backed out for causing bug 782981 on the latest Nightlies: https://hg.mozilla.org/mozilla-central/rev/7d4268f8884c Have triggered a Nightly respin.

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.

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

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

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?

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

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?

There's a whole raft of B2G video issues at the moment, so you're probably seeing one of them.