Last Comment Bug 784837 - call SetPrimaryFrame earlier during nsSubDocumentFrame::Init, so that container view can be found in order to reinitialize device context properly
: call SetPrimaryFrame earlier during nsSubDocumentFrame::Init, so that contain...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: unspecified
: x86_64 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks: 674373 775965
  Show dependency treegraph
 
Reported: 2012-08-22 15:10 PDT by Chris Pearce (:cpearce)
Modified: 2012-08-24 20:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, call SetPrimaryFrame earlier during nsSubDocumentFrame::Init (1.75 KB, patch)
2012-08-23 12:52 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-08-22 15:10:14 PDT
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) 2012-08-22 20:51:27 PDT
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) 2012-08-23 03:11:39 PDT
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) 2012-08-23 11:05:15 PDT
(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) 2012-08-23 12:03:00 PDT
(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) 2012-08-23 12:52:21 PDT
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.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-23 17:06:09 PDT
(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) 2012-08-23 17:08:42 PDT
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 9 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:03:40 PDT
https://hg.mozilla.org/mozilla-central/rev/5f8728a398a9

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