974616 - Window content doesn't rescale properly moving between Retina and non-Retina display with hardware acceleration off

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug is spun off from bug 962528 comment #10 and following, because it's a separate and unrelated bug.

Starting with the 2014-02-12-03-02-01-mozilla-central nightly, if "use hardware acceleration when available" is unchecked, moving a Firefox window between a Retina and a non-Retina display causes it to rescale incorrectly.  Instead of changing to the correct display mode, the contents actually changes size (inside a same-sized window).  So the content doubles in height and width moving from a non-Retina to a Retina display, and halves in height and width moving from a Retina to a non-Retina display.

Unlike with bug 962528, this effect is "permanent" -- it doesn't get corrected until you explicitly force the window to redraw (for example by resizing it).

This bug was triggered by the following patch:

https://hg.mozilla.org/mozilla-central/rev/08b4b4b4aef3

user:        Bill McCloskey <[e-mail]>
date:        Tue Feb 11 09:01:08 2014 -0800
summary:     Bug 960783 - Support "new out-of-process window" menu item in nightly (r=felipe,bsmedberg,trevor,ted)

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Window dragged from non-Retina to Retina display

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Window dragged from non-Retina to Retina display

Taken from Irving Reid's attachment 8377609 [details].

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Window dragged from Retina to non-Retina display

Taken from Irving Reid's attachment 8377611 [details].

Comment 4

[Steven Michaud [:smichaud] (Retired)]

The bug doesn't happen if you set both browser.tabs.remote and browser.tabs.remote.autostart to false.  So, though the bug can happen with what's supposedly a non-e10s window, I strongly suspect e10s code is involved.

I'll try to find exactly which code.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Benoit Girard tells me that in what I've called an e10s window (one where each tab's content is displayed by a content process), the tab's "title" is underlined for each tab that actually has content.

Normally a tab's title isn't underlined.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Markus, do you have any idea what's going on here?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I've actually found a fix for this.  Turns out we were mistakenly using the Basic compositor for a non-e10s window.

Bill, I've asked you to review because you're the author of the patches for bug 960783.  If you'd prefer to pass the review to someone else, please do so.

I've started an all-platform tryserver build.  The results should be available here by tomorrow morning:

https://tbpl.mozilla.org/?tree=Try&rev=c2838d8c16e3

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> https://tbpl.mozilla.org/?tree=Try&rev=c2838d8c16e3

There were no non-spurious test failures, except possibly for some of the Linux Opt Ripc tests.  I ran that set of tests twice and got identical failures in some reftests.  Since my patch can change which compositor gets used, these failures might be related to my patch.

It's possible I'll need to ifdef my patch to only effect certain platforms, or only OS X.

Here's a Mac tryserver build to test with:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c2838d8c16e3/try-macosx64/firefox-30.0a1.en-US.mac.dmg

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

Thanks Steve. Unfortunately, this break electrolysis on platforms without accelerated layers. I've posted an alternate patch in bug 974007 that should fix the problem.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> this break electrolysis on platforms without accelerated layers

I think you misunderstood my patch.  As best I can tell, it only changes our behavior for non-e10s windows, on any platform.

But I'll try your patches at bug 974007.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #10)
> > this break electrolysis on platforms without accelerated layers
> 
> I think you misunderstood my patch.  As best I can tell, it only changes our
> behavior for non-e10s windows, on any platform.
> 
> But I'll try your patches at bug 974007.

Let's say we open a new window in an e10s browser. The window widget lives in the parent process, and it needs to decide whether to enable OMTC. Your patch makes it so that we skip over the basic compositor in that case. If there's no accelerated compositor, then we won't get OMTC at all for that window. Later on, the child process will try to create its puppet widget and it will try to use the basic compositor. But by that point, OMTC has already been disabled for the window and it's too late.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> The window widget lives in the parent process

I didn't know this.  From glancing through the code I assumed it lived in the child process.

I stand corrected.  Thanks for the info.

How do you tell if a given window widget is for an e10s window?  In other words, how can you tell if a given window widget will have puppet widget children (in the e10s child process)?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #12)
> How do you tell if a given window widget is for an e10s window?  In other
> words, how can you tell if a given window widget will have puppet widget
> children (in the e10s child process)?

The second patch in bug 974007 adds a flag to nsBaseWidget to do that. It's based off the nsIWebBrowserChrome::CHROME_REMOTE_WINDOW flag on the chrome window.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Steve, can you please confirm that this is fixed now?

Comment 15

[Steven Michaud [:smichaud] (Retired)]

I tested today's mozilla-central nightly (on OS X 10.7.5), and yes, this bug is fixed there (presumably by the patches for bug 974007).

Comment 16

[:Irving Reid (No longer working on Firefox)]

Works for me on OS X 10.8.5 too - the flickering from bug 962528 is pretty bad, but the content ends up the correct size.