838239 - [hidpi] wrong devicePixelRatio in browser history

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[chrille_b]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130114 Firefox/20.0
Build ID: 20130114042016

Steps to reproduce:

Use mixed lo/hi-dpi screens with a macbook retina.
Open firefox on a hi-dpi screen.
Open a website (e.g. www.google.com) and search for something or click on a link.
Move firefox window to a lo-dpi screen.
Click on the back button.


Actual results:

The window has the wrong devicePixelRatio. It has the value of 2 (which was the value of my hi-dpi screen when the page was loaded at that screen).

This happens on firefox 20+ (Aurora and Nightly).


Expected results:

devicePixelRatio should have a value of 1.

Comment 1

[Jonathan Kew [:jfkthame]]

The reverse case (moving the window from lo-dpi to hi-dpi and then stepping back through history) also shows incorrect scaling.

Oddly, not -all- pages exhibit the problem; sometimes <back> gives me a properly-scaled page, but then going back another step may produces the incorrect scaling.

Anyway, there's definitely a problem here; it looks like when we restore content from the bfcache to the window, we don't check/update the pix ratio as needed.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Jonathan asked me for help with this while he was otherwise occupied, last week.  He pointed out that it'd be logical to call nsIPresShell::BackingScaleFactorChanged() from nsDocShell::RestoreFromHistory(), but that more seems to be needed.

I banged my head against this for a few days, last week and this week, and I think I've now come up with the answer.

The reason why a simple call to nsIPresShell::BackingScaleFactorChanged() didn't work is that the nsDocShell in question always corresponds to an nsChildView widget (i.e. to an nsView), not to an nsCocoaWindow widget (i.e. an nsWebShellWindow).  But if the backing scale factor of one needs to be updated, so does the other.

(Calls to BackingScaleFactorChanged() from widget code always happen for both kinds of object at the same time.  So we don't have to worry about this when, say, the user drags a window from a Retina display to a non-Retina display.)

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Jonathan, what do you think of this patch?

I suppose I should ask Roc to review it, but I'd like to get your opinion first.

In my tests it seems to do the trick.  I'm about to start a tryserver build, which (along with the test results) should be available in a few hours ... or at least by tomorrow morning.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 716848 [details] [diff] [review]
Fix

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

This seems to work fine in my testing - thanks!

I did wonder whether we could -always- use the "force root update" approach, and then dispense with the code that watches for backing scale factor changes on nsCocoaWindow, as it would be triggered by the child view. Might make things a bit simpler overall - WDYT?

::: layout/base/nsPresContext.cpp
@@ +1677,5 @@
> +{
> +  mPendingUIResolutionChanged = false;
> +
> +  mDeviceContext->CheckDPIChange();
> +  if (uint32_t(mCurAppUnitsPerDevPixel) != AppUnitsPerDevPixel()) {

You can drop the typecast here since bug 842514 landed. :)

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> I did wonder whether we could -always- use the "force root update" approach,
> and then dispense with the code that watches for backing scale factor
> changes on nsCocoaWindow, as it would be triggered by the child view. Might
> make things a bit simpler overall - WDYT?

Forget I said that, I don't think it's useful (it won't reach up into the chrome).

Comment 6

[Steven Michaud [:smichaud] (Retired)]

>> I did wonder whether we could -always- use the "force root update"
>> approach, and then dispense with the code that watches for backing
>> scale factor changes on nsCocoaWindow, as it would be triggered by
>> the child view. Might make things a bit simpler overall - WDYT?
>
> Forget I said that, I don't think it's useful (it won't reach up
> into the chrome).

It's also inefficient.  aForceRootUpdate actually forces a call to
AppUnitsPerDevPixelChanged() in the root presentation shell, whether
or not rootPC->mCurAppUnitsPerDevPixel ==
rootPC->AppUnitsPerDevPixel().  And there can be more than one
nsChildView per nsCocoaWindow (if the page includes one or more
plugins).

This forcing was deliberate, and was necessary to deal with the
following problem:

At first I just found the root shell and called
BackingScaleFactorChanged() on it from
nsDocShell::RestoreFromHistory().  And this actually worked in last
week's trunk code.  But when I refreshed my clone early this week, I
found that it no longer worked (that some recent change had broken
it).

In retrospect, it actually *shouldn't* have worked.  The native
window's backing scale factor will normally already have been changed
to the correct value by the time nsDocShell::RestoreFromHistory() is
called (this should have happened when the user moved the window from
one monitor to the other).  Which means
rootPC->AppUnitsPerDevPixelChanged() won't get called again unless we
somehow force it.

I don't fully understand why the second call to
rootPC->AppUnitsPerDevPixelChanged() is needed.  But my tests show
that it is.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (follow Jonathan's suggestion)

Carrying forward Jonathan's feedback+

Comment 8

[Jonathan Kew [:jfkthame]]

Once we have a reviewed-and-landed patch here, we should uplift it to aurora-21 and beta-20 if at all possible, so that it goes out with the rest of hidpi/multiscreen support.

Comment 9

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

This doesn't seem right to me. I expect to see code added that just updates the device context for the document that was retrieved from the bfcache, and its descendant documents. I don't know why we have to go back to the root.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

It's quite clear that we *do* have to go back to root, and that we have to call rootPC->AppUnitsPerDevPixelChanged() a "second" time ... or some part of that method.  But I don't fully understand why, either.  Do you have any ideas?

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> It's quite clear that we *do* have to go back to root ...

At least on OS X, which is so far the only place I've tested.

Do we have working HiDPI support on Windows or Linux, and is there a practical way to test it?

Comment 12

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

(In reply to Steven Michaud from comment #11)
> Do we have working HiDPI support on Windows or Linux, and is there a
> practical way to test it?

Not for dynamic changes, no.

(In reply to Steven Michaud from comment #10)
> It's quite clear that we *do* have to go back to root, and that we have to
> call rootPC->AppUnitsPerDevPixelChanged() a "second" time ... or some part
> of that method.  But I don't fully understand why, either.  Do you have any
> ideas?

No, I'm afraid not.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: handle possible resolution change when restoring content from history

Here's my latest attempt to fix this - seems to work in initial testing, at least.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 725115 [details] [diff] [review]
handle possible resolution change when restoring content from history

Looks fine to me.  It seems to be functionally equivalent to my patch, while being a lot simpler.

Comment 15

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

Comment on attachment 725115 [details] [diff] [review]
handle possible resolution change when restoring content from history

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

::: layout/base/nsPresContext.cpp
@@ +1680,5 @@
>    if (mCurAppUnitsPerDevPixel != AppUnitsPerDevPixel()) {
>      AppUnitsPerDevPixelChanged();
> +    nsPresContext* rootPC = GetRootPresContext();
> +    if (rootPC && rootPC != this && rootPC->mShell) {
> +      rootPC->mShell->ReconstructFrames();

I still don't understand why we have to reconstruct frames for the entire browser window.

Comment 16

[Jonathan Kew [:jfkthame]]

If we don't, the content that was restored from history into the window is scaled correctly, but its view fills only the top-left quarter of the window (if the move was from hidpi to lodpi) or extends to double the window's width and height (if vice-versa).

Presumably that's because it uses its "old" dpi to size itself when it is being inserted into the window, or something like that.

Calling ReconstructFrames on the root shell fixes that. Maybe there'd be a lighter-weight way to fix it, but so far I haven't found it. We're open to suggestions of where to poke at it...

Comment 17

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

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Presumably that's because it uses its "old" dpi to size itself when it is
> being inserted into the window, or something like that.
> 
> Calling ReconstructFrames on the root shell fixes that. Maybe there'd be a
> lighter-weight way to fix it, but so far I haven't found it. We're open to
> suggestions of where to poke at it...

I'd like to actually understand what the bug is before we work around it.

I'm pretty sure you should call BackingScaleFactorChanged() before Thaw(), for one thing. In fact I think basically we should call BackingScaleFactorChanged() as early as possible during the restoration process, as soon as we've obtained the shell. I'm suspicious of the oldBounds/newBounds stuff being wrong.

Comment 18

[Jonathan Kew [:jfkthame]]

I've already experimented with moving the BackingScaleFactorChanged() call earlier in RestoreFromHistory() (immediately after we get the shell), but it didn't help any.

Comment 19

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

OK, but it's really important we understand what this bug is and fix it properly instead of working around it.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: check for possible DPI change when restoring content from browser history

The reason simply calling shell->BackingScaleFactorChanged() wasn't sufficient is that nsPresContext uses a runnable to actually implement the update, so this call does not cause the device context's appUnitsPerDevPixel to be immediately updated - it will happen later on the event loop. But this means that it is still out-of-date at the point where we call mContentViewer->SetBounds to adjust the size of the restored viewer, and hence we get it wrong. This version of the patch fixes this by explicitly calling CheckDPIChange on the dev context, so that its appPerDev value will be correct by the time we rely on it for sizing the widget. (An alternative solution would be to make nsPresContext::UIResolutionChanged() work synchronously, instead of via a runnable; I've confirmed that would also fix the problem, but it's probably less desirable overall.)

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 736769 [details] [diff] [review]
check for possible DPI change when restoring content from browser history

Looks good to me.  Nice catch!

Comment 22

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

Comment on attachment 736769 [details] [diff] [review]
check for possible DPI change when restoring content from browser history

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

Great, thanks!

Comment 23

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd472d11b4c

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1fd472d11b4c

Comment 25

[Jonathan Kew [:jfkthame]]

While this is a relatively obscure issue - it only occurs when a user moves the browser window between Retina and non-Retina screens, and then moves backward or forward through cached pages - the result is pretty ugly.

Given how simple and safe the fix is, I think we should uplift this to fix the regression for Retina-plus-external-display users.

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 736769 [details] [diff] [review]
check for possible DPI change when restoring content from browser history

[Approval Request Comment]
Bug caused by (feature/regressing bug #): HiDPI (Retina) display and OS X multiple screen support

User impact if declined: bad display (double- or half-size web content) after moving window between screens and then clicking Back or Forward to go to a page from the bfcache.

Testing completed (on m-c, etc.): several days on Nightly

Risk to taking this patch (and alternatives if risky): minimal risk - localized patch that has no effect unless there's a DPI mismatch, and in that case just calls existing resolution-update code

String or IDL/UUID changes made by this patch: none

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 736769 [details] [diff] [review]
check for possible DPI change when restoring content from browser history

The patch looks contained and safe and it helps avoid bad display issues for users using mixed low/high-DPI screens .Will back-out if we see any new regressions off this.

:jfkthame, I am going to request some exploratory testing around this from QA, but any local testing you can further do to be more confident will be very helpful in case you have the set-up ready.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to chrille_b from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0)
> Gecko/20130114 Firefox/20.0
> Build ID: 20130114042016
> 
> Steps to reproduce:
> 
> Use mixed lo/hi-dpi screens with a macbook retina.
> Open firefox on a hi-dpi screen.
> Open a website (e.g. www.google.com) and search for something or click on a
> link.
> Move firefox window to a lo-dpi screen.
> Click on the back button.
> 
> 
> Actual results:
> 
> The window has the wrong devicePixelRatio. It has the value of 2 (which was
> the value of my hi-dpi screen when the page was loaded at that screen).
> 
> This happens on firefox 20+ (Aurora and Nightly).
> 
> Expected results:
> 
> devicePixelRatio should have a value of 1.

Hi chrille_b,

Can you please try our latest nightly to see if the issue is fixed for you ?

Thanks for reporting the issue and hope this is resolved for you now.

Comment 29

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29579236eefc
https://hg.mozilla.org/releases/mozilla-beta/rev/a57fd32d0e28

Comment 30

[Jonathan Kew [:jfkthame]]

(In reply to bhavana bajaj [:bajaj] from comment #27)
> :jfkthame, I am going to request some exploratory testing around this from
> QA, but any local testing you can further do to be more confident will be
> very helpful in case you have the set-up ready.

The key elements needed to reproduce the bug are (a) Mac with a mix of Retina and non-Retina screens; (b) navigating backwards and forwards among cached pages in the bfcache; and (c) interspersed with that, moving the browser window between screens with differing resolution. In this way, I was consistently able to trigger the incorrect display for content reloaded from the cache.

Once the patch was applied, I have been unable to reproduce it in repeated testing. So for my configuration, at least, I'm pretty confident the issue is resolved. Additional QA testing is of course welcome.

Comment 31

[u279076]

Matt, can you please verify this is fixed? It should currently be fixed in Firefox Nightly 23.0a1 builds and should be fixed in tomorrow's Aurora and the next Beta.

Comment 32

[chrille_b]

(In reply to bhavana bajaj [:bajaj] from comment #28)
> (In reply to chrille_b from comment #0)
> > User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0)
> > Gecko/20130114 Firefox/20.0
> > Build ID: 20130114042016
> > 
> > Steps to reproduce:
> > 
> > Use mixed lo/hi-dpi screens with a macbook retina.
> > Open firefox on a hi-dpi screen.
> > Open a website (e.g. www.google.com) and search for something or click on a
> > link.
> > Move firefox window to a lo-dpi screen.
> > Click on the back button.
> > 
> > 
> > Actual results:
> > 
> > The window has the wrong devicePixelRatio. It has the value of 2 (which was
> > the value of my hi-dpi screen when the page was loaded at that screen).
> > 
> > This happens on firefox 20+ (Aurora and Nightly).
> > 
> > Expected results:
> > 
> > devicePixelRatio should have a value of 1.
> 
> Hi chrille_b,
> 
> Can you please try our latest nightly to see if the issue is fixed for you ?
> 
> Thanks for reporting the issue and hope this is resolved for you now.

Hi,

yes, I can confirm that the issue is fixed for me in the latest nightly build 23.0a1 (2013-04-20) and the latest aurora build 22.0a2 (2013-04-20).

Thank you all for resolving the issue.

Comment 33

[u279076]

Thank you chrille_b. We should have a new beta build with this fix by mid-week.

Comment 34

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed broken 2013-02-05, builds 20 and up.
Confirmed fixed 2013-04-22, 22.0a2 and 23.0a1.

Comment 35

[u279076]

(In reply to Matt Wobensmith from comment #34)
> Confirmed broken 2013-02-05, builds 20 and up.
> Confirmed fixed 2013-04-22, 22.0a2 and 23.0a1.

Thanks Matt. We now have Firefox 21.0b4 candidate builds. Can you please verify this is fixed in Beta?

ftp://ftp.mozilla.org/pub/firefox/nightly/21.0b4-candidates/build1/

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Anthony.

Confirmed fixed 21.0b4.

Comment 37

[u279076]

Much appreciated Matt.