978913 - [e10s] Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Chris Peterson [:cpeterson]]

I have a Retina MacBook Pro with a non-HiDPI external display. When I drag an e10s window from the external display to the MacBook Pro's Retina display, the page text becomes very tiny. If I drag an e10s window from the Retina display to the external display, the page text becomes very large.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This can happen even with a non-e10s window, thanks to a bug in e10s code.  See bug 974616.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Actually bug 974616 has now been fixed (by the patches for bug 974007).

Comment 3

[Chris Peterson [:cpeterson]]

I can still repro this bug with an e10s window in Nightly 30 build 2014-03-04. Non-e10s windows work correctly.

Comment 4

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

Are there any kind of notifications that fire when a window is dragged across monitors? Maybe we're just not passing the notification to the content process.

Comment 5

[Mike Hoye [:mhoye]]

I can reproduce this bug as of 31.0a1 (2014-03-19).

Comment 6

[Mike Conley (:mconley) (:⚙️)]

The event that billm is referring to is windowDidChangeScreen, which is received by nsCocoaWindow, and fires mGeckoWindow->BackingScaleFactorChanged();.

BenWa helped me figure that one out, and we determined that the child process is definitely not having BackingScaleFactorChanged on it, so we might need to forward this to the child.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

So here's what I've found out:

When an nsCocoaWindow hears windowDidChangeScreen, it calls its own BackingScaleFactorChanged function, which in turn calls nsPresContext::UIResolutionChanged for the associated nsPresContext, which runs this function in a runnable:

void
nsPresContext::UIResolutionChangedInternal()
{
  mPendingUIResolutionChanged = false;

  mDeviceContext->CheckDPIChange();
  if (mCurAppUnitsPerDevPixel != AppUnitsPerDevPixel()) {
    AppUnitsPerDevPixelChanged();
  }

  mDocument->EnumerateSubDocuments(UIResolutionChangedSubdocumentCallback,
                                   nullptr);
}

That UIResolutionChangedSubdocumentCallback is this:

/*static*/ bool
nsPresContext::UIResolutionChangedSubdocumentCallback(nsIDocument* aDocument,
                                                      void* aData)
{
  nsIPresShell* shell = aDocument->GetShell();
  if (shell) {
    nsPresContext* pc = shell->GetPresContext();
    if (pc) {
      pc->UIResolutionChangedInternal();
    }
  }
  return true;
}

I think this is a spot (hopefully the right one) where we can call the TabParents for all of these documents, to signal to their TabChildren to call UIResolutionChanged on all of their nsIDocument's nsPresContexts.

So, the question is: is it possible to somehow go from each nsIDocument passed to this UIResolutionChangedSubdocumentCallback to their respective TabParents? If so, how?

billm, do you know?

Comment 8

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

I don't know anything helpful here. I don't see any easy way to find all the TabParents for a given window. The message manager keeps that sort of per-window data, but it doesn't look easy to get out what you need. Another option would be to enumerate over all TabParents and filter out the ones that are in the window that moved. That's not all that easy either though.

Maybe smaug has a better idea?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Talked to smaug about this yesterday, and what I'm going to try is to get to the XUL document when the UIResolutionChanged method is called. From there, get to the "window" object, and then try to broadcast via window.messageManager to all of the remote browsers to update their nsPresContexts as well.

I'm seeing a lot of message manager use via from JS, but fewer examples in C++, but it _seems_ like it should be possible.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Clearing needinfo on smaug for now, based on above.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

So originally, I was trying to iterate the subdocuments inside nsPresContext::UIResolutionChangedSubdocumentCallback to trace them to their respective TabParents to send the UIResolutionChanged signal to the children.

Turns out that's really not easy - in fact, I found some other code that uses a hack to cast the callback passed to the nsFrameMessageManager in its Init call back into the nsFrameLoader, from which we can get to the TabParent.

That code is here: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsCCUncollectableMarker.cpp#138

My suggestion is to use the same hack, and then to file a new bug to make it easier to go from an nsIMessageListenerManager back to a nsFrameLoader.

That's what I'm doing here with this WIP, and I also have to invalidate some caches, because TabParent and TabChild both cache the DPI and default scales it seems.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8426663 [details] [diff] [review]
WIP Patch 1

Am I on the right track here?

Comment 13

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

Comment on attachment 8426663 [details] [diff] [review]
WIP Patch 1

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

This seems fine to me.

::: layout/base/nsPresContext.cpp
@@ +1771,5 @@
>    if (mCurAppUnitsPerDevPixel != AppUnitsPerDevPixel()) {
>      AppUnitsPerDevPixelChanged();
>    }
>  
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

Do we need this check? I didn't think we had a chrome window in the child process, but I could be wrong. Maybe you could just turn it into a MOZ_ASSERT.

@@ +1786,5 @@
> +        windowMM->GetChildAt(j, getter_AddRefs(childMM));
> +        if (!childMM) {
> +          continue;
> +        }
> +        nsCOMPtr<nsIMessageSender> strongTabMM = do_QueryInterface(childMM);

I think if you use strongTabMM.get() in the cast below, you won't need separate strong and weak variables.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly. r=?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8427090 [details] [diff] [review]
Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly. r=?

Thanks for the feedback+, billm!

Addressed feedback, and cleaned it up some. Ready for a real review.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8427090 [details] [diff] [review]
Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly. r=?

>+  virtual void ClearBackingScaleCache() {
Nit, { goes to its own line

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Nit fixed! Thanks smaug!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f1068bf18e64

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage on most platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad797cc2147a

https://tbpl.mozilla.org/php/getParsedLog.php?id=40212911&tree=Mozilla-Inbound

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Ok, found the issue - needed to bring in nsFrameLoader.h for non-desktop builds, I guess.

Here's a try push:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=590ac758bbd9

Weird to have to request so many tests and systems to run, but, well, that's platform I guess.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Ok, I think it's landable now with that missing include.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a40b1e15ae8b

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a40b1e15ae8b