The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
Layout: View Rendering
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: jfkthame)

Tracking

unspecified
mozilla17
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

5 years ago
Created attachment 654748 [details] [diff] [review]
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.
(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 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.
Attachment #654748 - Flags: review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8728a398a9
Assignee: nobody → jfkthame
Summary: Prevent nsSubDocumentFrame presentation restore on reframe if device context is incompatible → call SetPrimaryFrame earlier during nsSubDocumentFrame::Init, so that container view can be found in order to reinitialize device context properly
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/5f8728a398a9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.