784837 - call SetPrimaryFrame earlier during nsSubDocumentFrame::Init, so that container view can be found in order to reinitialize device context properly

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

From bug 775965, comment 24:

(In reply to Jonathan Kew (:jfkthame) from comment #24)
> [bug 775965] 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?

We can do this check when we re-attach the stashed presentation in nsSubDocumentFrame::Init(), in the same place where we do the "oldContainerDoc == aContent->OwnerDoc()" check.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonathan, how could a presentation get initialized for a non-HiDPI context and then get restored into a HiDPI context? Isn't normally every presentation initialized for a HiDPI context when you're running Firefox on a single monitor with HiDPI enabled?

Comment 2

[Jonathan Kew [:jfkthame]]

I'm not sure exactly what's happening yet... just noting the observed symptoms, and the fact that discarding the stashed presentation prevents the problem.

After a bit of poking around, AFAICT it seems the dev context of the stashed presentation does have the correct appUnitsPerDevPixel, so it's not as simple as a mismatch there. I'll try to come up with a reduced example to investigate further...it may be the problem is at a different level.

I still wonder if there isn't a latent issue here, though - what's to prevent the user moving a window between HiDPI and non-HiDPI screens while there's a detached/stashed presentation?

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> Jonathan, how could a presentation get initialized for a non-HiDPI context
> and then get restored into a HiDPI context? Isn't normally every
> presentation initialized for a HiDPI context when you're running Firefox on
> a single monitor with HiDPI enabled?

Not necessarily, at the moment (though we could change that). In nsDeviceContext::SetDPI, the device context gets the ratio of device pixels to screen points from mWidget; if there is no widget, it defaults to 1.0 (i.e. "non-HiDPI"). We could change that so that it defaults to 2.0 (HiDPI mode), and then the problem here with certain stashed presentations being mis-sized does indeed go away on my HiDPI screen - but it will be liable to appear (in reverse) when running on a non-HiDPI screen.

It's perfectly possible for a system to have both Hi- and Lo-DPI displays, so I don't think it's feasible for us to guarantee that the device context of a "detached" presentation (without associated widget) will always be initialized correctly for wherever it may eventually be used.

After digging deeper here with the Google "+1" button as testcase, I've determined that the stashed presentation actually did have the right resolution, AFAICT; the problem arises later, when EndSwapDocShellsForDocument() reinitializes the device context, at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#1018. In the case where there is no container view, it passes null as aWidget to nsDeviceContext::Init(), which therefore cannot determine the proper screen point to device pixel ratio, and sets it to the default of 1.0 (see above).

A workaround for common cases, at least, is to make the dev ctx leave its point/pixel ratio unchanged if no widget was available. However, if there's any possibility of a detached subdocument presentation being moved between contexts with different resolutions, it still seems to me that there's a potential issue here. Maybe we can defer the reinitialization of the dev ctx to a later time when the necessary widget is available?

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

> A workaround for common cases, at least, is to make the dev ctx leave its
> point/pixel ratio unchanged if no widget was available. However, if there's
> any possibility of a detached subdocument presentation being moved between
> contexts with different resolutions, it still seems to me that there's a
> potential issue here.

Yup - this is definitely inadequate; the G +1 button, GMail frame, etc end up incorrectly scaled after dragging the window between HiDPI and non-HiDPI displays, even though they display properly when initially loaded (on either display).

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, call SetPrimaryFrame earlier during nsSubDocumentFrame::Init

It seems like the problems I'm seeing here with device context initialization lacking a valid widget can be solved by calling SetPrimaryFrame earlier during nsSubDocumentFrame::Init, so that we're able to find a container view when inserting the stashed presentation. I'm not sure yet whether this might have other (unwanted) effects... let's see what the test suites think about it.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> I still wonder if there isn't a latent issue here, though - what's to
> prevent the user moving a window between HiDPI and non-HiDPI screens while
> there's a detached/stashed presentation?

The presentation is only stashed during the execution of a single XPCOM event (it's unstashed off a scriptrunner, so is only stashed while it's not safe to run scripts). So you can't stash a presentation, move a window somewhere else, and then unstash it.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654748 [details] [diff] [review]
patch, call SetPrimaryFrame earlier during nsSubDocumentFrame::Init

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

Looks good to me. This will make sure you find the widget you need.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8728a398a9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f8728a398a9