<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Lawrence Mandel [:lmandel] (use needinfo)

Reporter

•

11 years ago

Turns out I can't reproduce this with a fresh profile, or after restarting 37.0b2. There's a graphical glitch when transferring the window between displays, but then the window is rerendered afterwards and looks correct. Perhaps it's a state which develops after running for some time.

Comment 10

•

11 years ago

I can reproduce this reliably on my machine using 37 Beta 2 and Nightly 39. This occurs every time I plug in the monitor at my desk. I filed bug 1118290, which may be a dup of this one. Tracking as this is quite annoying and reflects poorly on the browser. (Content renders incorrectly.) Given that other people are seeing this, I would prefer to have a fix. However, I will also say I don't think this is a stop ship issue as there is a simple workaround.

tracking-firefox37: ? → +

tracking-firefox38: --- → +

tracking-firefox39: --- → +

David Burns :automatedtester

Comment 11

•

11 years ago

I can reproduce this too on Firefox 38 and Firefox 39. see https://www.evernote.com/shard/s63/sh/4b92ee0b-aadb-475b-9524-c5a302346d06/5c37ab2aaa8da81ac206ad4ffc2512d2

Comment 12

•

11 years ago

We can't make any headway here until we have STR good enough for *everyone* to be able to reproduce it, close to 100% of the time, starting from a clean profile.

Milan Sreckovic [:milan] (needinfo for best results)

Comment 13

•

11 years ago

I suspect we'll only be able to fix this bug by fixing the temporary glitches and artifacts. Safari never had them, of course (thanks to it's knowledge of OS X internals). And fairly recently Chrome stopped having them (see bug 962528 comment #33). So in principle we should be able to do the same. But it will be a *major* project, and require *lots* of reverse engineering (of Safari, Chrome, and OS X). I'm not sure I'll ever have time for it. If/when I do, though, I'll probably work on it at bug 962528.

Comment 14

•

Comment 26

•

11 years ago

I can reliably reproduce this on my laptop and external monitor. Although the sizing is incorrect for just a flicker, the browser content immediately repaints to correct size. Most easily seen with about:home. - Move browser from laptop to external monitor: a larger than expected content flickers once. - Move browser from external monitor to laptop screen; a smaller than expected content flickers once. I'll bisect for a regression window.

Flags: needinfo?(twalker)

Tracy Walker [:tracy]

Comment 27

•

11 years ago

This has been around since at least Jan. 1, 2014. It doesn't appear to me to be e10s isolated, as I can reproduce (in variations) with current Nightly, current release and old Nightlies with e10s disabled. I noticed that at times (upon moving the browser from one screen to the other) it seems to flicker content from some other source, perhaps the cache? (probably should be noted I am running Nightly and release side by side)

Flags: needinfo?(twalker)

John Ford [:jhford] CET/CEST Berlin Time

Comment 28

•

11 years ago

Tracy, don't bother to try to find a regression range for the temporary artifacts. They've been around since we starting supporting HiDPI mode. However, we really do need someone to try to find a regression range (or ranges) for the "permanent" artifacts (those which only disappear after a manually triggered refresh). This will, of course, need to be someone who can reproduce them.

Comment 29

•

Comment 48

•

11 years ago

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #45) > hmmm, I can't reproduce this in a completely fresh profile. Check that, I can actually reproduced this with a fresh profile on Latest Nightly. The critical step, for me to be able to reproduce this, is putting the browser in full screen mode after the system has moved the browser to the laptop screen when you disconnect the external monitor. (step #3)

Tracy Walker [:tracy]

Comment 49

•

11 years ago

When I say Fullscreen, I mean use the expand arrows in upper right corner of the browser, not the Mac green max window size button. my System info: MacBook Pro Retina, 15-inch, Late 2013 Processor 2.3 GHz Intel Core i7 Memory 16 GB 1600 MHz DDR3 Graphics NVIDIA GeForce GT 750M 2048 MB Software OS X 10.9.5 (13F34)

Comment 50

•

11 years ago

> step #3 Which comment does this refer to? Better still, please write out your full STR in detail.

Tracy Walker [:tracy]

Comment 51

•

11 years ago

STR

Pretty much as in comment #11. But I'll spell out the STR's with a bit more detail. setup: - Modern MacBook Pro laptop and external monitor (I have 25" HP) - In System Prefs > Displays: -- Display: Both screens have "Best for display" selected. -- Arrangement: External monitor is directly above laptop screen. External display has the Menu Bar. Mirror Displays is not selected. - Have latest build of Nightly running with the browser window in the external monitor. I just run with about:home loaded in a single tab. STR's: 1. Disconnect the external monitor by removing HDMI cable from the Macbook port. 2. Wait for the browser to appear on Macbook display. 3. Then, resize Nightly to Full Screen mode (use expand arrows in upper right corner of browser window or shift+command+f) 4. Close the Macbook so it goes to sleep 5. Reconnect the external monitor by plugging the HDMI cable back into the external display port. 6. Open the Macbook so it wakes up and wait for Nightly to appear on the external monitor. tested results: The browser content is about 1/4th of the expected size and is located in the upper left corner of the content pane. note: sometimes, but less reliably than the full steps, after step 2, the content of about:home appears enlarged. expected results: Normal sized content.

Comment 52

•

11 years ago

Yes! Thanks to you, Tracy, I've now got working STR! As you said, it's crucial to go to fullscreen mode in step 3 (not just to "resize firefox to be maximum on Laptop screen", which must reasonably be interpreted as zooming up the Firefox browser window). I'll double-check your ranges from comment #32, and hopefully be able to find exactly which patches triggered those changes.

Comment 53

•

11 years ago

Interestingly, though, I can only reproduce in e10s mode.

Comment 54

•

11 years ago

Tracy, can you still reproduce in non-e10s mode? The fact that I can't means that I'm stuck once again. Your first range from comment #32 was also the range in which e10s support was first turned on as an option (bug 960783).

Flags: needinfo?(twalker)

John Ford [:jhford] CET/CEST Berlin Time

Comment 55

•

11 years ago

Tracy, I find that your first range from comment #32 is invalid in e10s mode. In m-c builds before 2014-02-13 you need to explicitly set browser.tabs.remote and browser.tabs.remote.autostart to true to get e10s mode. When I do that I see the "content too small" bug in the 2014-02-12 m-c nightly (and m-c nightlies much earlier than that). I suspect that bug goes back to the earliest days of e10s support.

Comment 56

•

11 years ago

(In reply to Steven Michaud from comment #54) > Tracy, can you still reproduce in non-e10s mode? The fact that I can't > means that I'm stuck once again. Your first range from comment #32 was also > the range in which e10s support was first turned on as an option (bug > 960783). fwiw, I can reproduce with latest release channel firefox, which doesn't have e10s. I've also started to see content going to 4x normal size on the retina display.

Comment 57

•

11 years ago

I really need to be able to reproduce in non-e10s mode with m-c nightlies.

Reporter

•

11 years ago

I am unable to reliably reproduce in non-e10s mode.

Flags: needinfo?(twalker)

Assignee

Updated

•

11 years ago

Attachment #8581735 - Flags: review?(bugs)

Comment 63

•

11 years ago

Comment on attachment 8581735 [details] [diff] [review] Detect stale display properties when resizing tabs So we'd send UIResolutionChange messages twice, once in TabParent::UpdateDimensions, and once in nsPresContext::UIResolutionChangedInternal(). Latter should be enough. Why isn't receiving UIResolutionChange once in TabChild enough? If the bug happens on non-e10s too, sounds like either we don't get UIResolutionChange even in the parent process when we should, or that AppUnitsPerDevPixelChanged() doesn't end up causing a new repaint.

Attachment #8581735 - Flags: review?(bugs) → review-

Assignee

Comment 64

•

11 years ago

AppUnitsPerDevPixelChanged doesn't initiate a repaint. That would also fix the bug since we have correct display stats at that point. I'll look into finding a way to request repaint in AppUnitsPerDevPixelChanged.

•

11 years ago

Attached patch Make TabParent/TabChild UpdateDimensions messages aware of the display scale (obsolete) — Details — Splinter Review

See comments 65-67. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55271b358b95

Attachment #8585809 - Attachment is obsolete: true

Attachment #8586866 - Flags: review?(roc)

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

Assignee

Comment 69

•

11 years ago

I should mention that the difference between the attachment in 67 and the one in 68 - that fixed the B2G failures - is mainly about AppUnitsPerDevPixel vs AppUnitsPerDevPixelAtUnitFullZoom.

Updated

•

11 years ago

Attachment #8586866 - Flags: review?(roc) → review+

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

Comment 70

•

11 years ago

(In reply to David Parks [:handyman] from comment #66) > The assessment in comment 65 was valid and the bugs have been dealt with. > With this patch, the display does an incorrect rendering after the > UpdateDimensions call and then a correct rendering after the > UIResolutionChanged call. This doesn't look too bad. Really? It sounds bad :-). What would it take to fix that glitch?

Assignee

Comment 71

•

11 years ago

What I meant by 'not that bad' is that the behavior seems usually to be invisible -- pretty often, the display stays dark until the new layout is available (admittedly, this is true on my old Dell monitor and may not be true in general). I'm not sure why the nsPresContext::UIResolutionChanged calls are made asynchronously: https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1774 I suspect its for performance in the case where the browser is dragged between monitors. So making this synchronous would also fix this bug (without the glitch) but would probably break... something. The alternative is to adapt either TabChild::RecvUpdateDimensions or TabChild::RecvUIResolutionChanged to ID this case and handle it. I'll try this in UpdateDimensions but it will take some bookkeeping. The TabParent can remember the last backingScale value it gave the child and, if it doesn't match the value in the widget when the SendUpdateDimensions call is happening, it can send a boolean to the child telling it to invalidate its backingScale cache value. (Since the async UIResolutionChanged calls happen, I believe, on the same thread, we won't end up in a race condition.) I could send the value but the child requests that from the parent by itself when the cache is invalidated (and there are potentially other things that happen so I won't reinvent it). This is a little involved but it wouldn't be fragile. This may be a better implementation -- it should work even without the current patch. I'll sit on the checkin until you have a chance to chime in so let me know what you prefer.

Flags: needinfo?(roc)

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

Assignee

Comment 72

•

11 years ago

Attached patch WIP: Alternative solution (obsolete) — Details — Splinter Review

So... I decided to try for the alternative. Its probably the better solution. But its not perfect... This patch does what I suggested in the last comment, and it fixes the original issue (without any graphics glitch in the STR), but it still has an additional glitch when going from the external display back to the laptop. The laptop screen wakes up much faster (I dont know if this is relevant). My total guess as to what is happening is that the compositor is still running while the UpdateDimensions stuff is happening and the compositor blits an image from the ext display's form factor and scale... so on the laptop I see a small version of the screen in a big window before it is corrected. So I'd need to stop the compositor when the displayChanged message came in and restart it... sometime? I guess start it asynchronously once the child processed its UpdateDimesions call. Assuming thats possible. But this is jumping the gun... I dont know if any of that is the cause. At any rate, I'd probably bug that rather than fix it right away. So, roc... preference?

Comment 73

•

11 years ago

Comment on attachment 8587139 [details] [diff] [review] WIP: Alternative solution Review of attachment 8587139 [details] [diff] [review]: ----------------------------------------------------------------- How about combining the UpdateDimensions and UIResolutionChanged calls, and always pass the UI resolution along with the dimensions? Then the other thing we should do, to avoid obvious glitching, is to label the TabChild's layer tree with the UI resolution used to generate it, and in the compositor, scale it if necessary. That means, if the content process doesn't paint in time for whatever reason, we still render something reasonable.

Brad Lassey [:blassey] (use needinfo?)

Comment 74

•

11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73) > Comment on attachment 8587139 [details] [diff] [review] > WIP: Alternative solution > > Review of attachment 8587139 [details] [diff] [review]: > ----------------------------------------------------------------- > > How about combining the UpdateDimensions and UIResolutionChanged calls, and > always pass the UI resolution along with the dimensions? > > Then the other thing we should do, to avoid obvious glitching, is to label > the TabChild's layer tree with the UI resolution used to generate it, and in > the compositor, scale it if necessary. That means, if the content process > doesn't paint in time for whatever reason, we still render something > reasonable. This sounds like it adds a lot of complexity, how about we land this roughly as is and do that in a follow up?

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

•

10 years ago

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 79

•

10 years ago

Hi, seems this failed to apply: adding 1125325 to series file renamed 1125325 -> extdisp.final2.patch applying extdisp.final2.patch patching file dom/ipc/TabChild.cpp Hunk #3 FAILED at 273 1 out of 10 hunks FAILED -- saving rejects to file dom/ipc/TabChild.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh extdisp.final2.patch could you take a look, thanks!

Flags: needinfo?(davidp99)

Keywords: checkin-needed

Assignee

Comment 80

•

10 years ago

Attached patch Make TabParent/TabChild UpdateDimensions messages aware of the display scale (r+ed by roc) (obsolete) — Details — Splinter Review

This should do it...

Attachment #8586866 - Attachment is obsolete: true

Attachment #8594834 - Attachment is obsolete: true

Flags: needinfo?(davidp99)

Assignee

Updated

•

10 years ago

Keywords: checkin-needed

•

10 years ago

Attached patch Make TabParent/TabChild UpdateDimensions messages aware of the display scale (obsolete) — Details — Splinter Review

This deals with the incorrect use of coordinate system typedefs in general and kats' two concerns specifically (the first directly, the second was an unnecessary variable that resulted from an hg merge). It also deals with the new issue I reported in comment 82. The problem was that the nsPresContext was asynchronously invalidating the scale/dpi. This should be unnecessary in the TabChild::RecvUIResolutionChanged IPDL message handler. I now invalidate the values synchronously there. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc39e5a846bd

Attachment #8595458 - Attachment is obsolete: true

Assignee

Comment 84

•

10 years ago

Comment on attachment 8597398 [details] [diff] [review] Make TabParent/TabChild UpdateDimensions messages aware of the display scale Tests look good. A quick check of B2G on Flame ran fine. And the coordinate systems should be lined up correctly.

Attachment #8597398 - Attachment description: Make TabParent/TabChild UpdateDimensions messages aware of the display scale (r+ed by roc) → Make TabParent/TabChild UpdateDimensions messages aware of the display scale

Attachment #8597398 - Flags: review?(bugmail.mozilla)

Comment 85

•

10 years ago

Comment on attachment 8597398 [details] [diff] [review] Make TabParent/TabChild UpdateDimensions messages aware of the display scale Review of attachment 8597398 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, this was hard to review for me. Based on my gut feeling I would r- this patch, but honestly I don't know enough about the UpdateDimensions call and the units of the variables being passed around to definitively point to something as wrong. I've braindumped my thoughts below so you can address what you want and land with roc's r+ if you like. ::: dom/ipc/TabChild.cpp @@ +3216,5 @@ > + nsCOMPtr<nsIDocument> document(GetDocument()); > + nsIPresShell* presShell = document ? document->GetShell() : nullptr; > + nsPresContext* presContext = presShell ? presShell->GetPresContext() : nullptr; > + presContext = presContext ? presContext->GetDisplayRootPresContext() : presContext; > + return presContext ? presContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom() : 1; You should probably MOZ_RELEASE_ASSERT things here so that you never actually fall through and return 1. If that ever happens then it's going to result in badness. ::: dom/ipc/TabChild.h @@ +221,5 @@ > + int32_t AppUnitsPerDevPixelAtUnitFullZoom(); > + > + ScreenIntSize GetInnerSizeInScreenUnits() { > + int32_t scale = AppUnitsPerDevPixelAtUnitFullZoom(); > + return ScreenIntSize(mInnerSize.width / scale, mInnerSize.height / scale); Not clear to me how you can convert from app units to ScreenPixels using AppUnitsPerDevPixelAtUnitFullZoom, because that converts from app units into a coordinate space that we haven't given a name. This needs some explanation. See my other comments later. Also given the function is returning a ScreenIntSize, the "InScreenUnits" part of the function name is redundant. I'd rather just have it as GetInnerSize(), and make sure that mInnerSize is a private variable so it can only be accessed via this getter. ::: dom/ipc/TabParent.cpp @@ +978,5 @@ > + int32_t appPerScreen = presContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom(); > + appPixelRect.ScaleRoundOut(appPerScreen); > + appPixelSize = appPixelSize * appPerScreen; > + } > + unused << SendUpdateDimensions(appPixelRect, appPixelSize, mOrientation, chromeOffset); So a few things here: - For consistency, please stick with using either the member variables or the local variables. For example you use |mRect| but also |size| (as opposed to mDimensions). The SendUpdateDimensions call also uses |mOrientation| with |chromeOffset| - I would prefer either |orientation| or |mChromeOffset|. - If presContext is nullptr, then I think you should just return instead of sending an update dimensions call with what amounts to garbage values (they will be sent in one coordinate space and interpeted in a different one). - "appPixelRect" and "appPixelSize" are not great names. There is no such thing as an "appPixel" - there's various kinds of pixels, and there's app units. I would prefer the names "appUnitsRect" and "appUnitsSize". - Since you're converting to app units, the types should be nsRect and nsSize, because those store app units. The nsInt* types are for pixels (typically LayoutDevicePixels, but depends on the particular use). PBrowser.ipdl should be updated to use nsRect and nsSize as well. - It's not clear to me why you're using AppUnitsPerDevPixelAtUnitFullZoom() instead of just AppUnitsPerDevPixel() here. Do the rect/size values change when full-zoom is applied? If so, please add a comment to that effect. - In bug 1144940, dzbarsky changes the |rect| argument to this function from nsIntRect to a ScreenIntRect. If he's correct and the value is in ScreenPixels (quite possibly not the case) then you'll need to justify a little bit better why you can convert it to app units using AppUnitsPerDevPixelAtUnitFullZoom, because it's not clear to me. If you haven't taken a look at the documentation in layout/base/Units.h you might want to do that.

Attachment #8597398 - Flags: review?(bugmail.mozilla)

Assignee

Comment 86

•

10 years ago

I will definitely re-ping roc in case he wants to refresh his r+ but first lets try to iron out a few things. The central problem is that we are updating data in the UpdateDimensions call that is combined with the data in the UIResolutionChanged call. The algebra of the patch works by separating the information in the UpdateDimensions message. In UIResolutionChanged, we update the scale (X) and in UpdateDimensions, we were updating the *scaled* display size (X*Y). During a period where we’ve updated X*Y without updating X, some calculations do ‘Y/X’ to get Y but they are using the wrong X. At its core, the change just passes Y instead of X*Y, which it computes using the cached X. So e.g. we were passing ScreenIntSize (as dzbarsky wrote) and I’ve changed it to app units. There were a few ways that this could be represented. I chose app units because they are widget-scale-agnostic but, rereading the coordinate stuff, I might have been able to use LayoutDevicePixel units. However, they are supposed to be widget-scale-sensitive [1]. So I went with app units. Since the app units describe the screen dimensions with no scale or zoom, AppUnitsPerDevPixelAtUnitFullZoom seemed like the right way to do the conversion. Additionally, AppUnitsPerDevPixel fails here — it’s readily apparent on B2G. We do use this function for screen coords (for example, Event::GetScreenCoords [2]). Let me know if there is a better way. The rest of your comments seem cosmetic and should be easy enough to fix. I'll put together a new patch if this sounds acceptable. [1] https://mxr.mozilla.org/mozilla-central/source/layout/base/Units.h#230 [2] https://mxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp#907

Flags: needinfo?(bugmail.mozilla)

Comment 87

•

10 years ago

Attached image Different widget areas — Details

Most of what you say makes sense. However my understanding of the widget-world coordinate systems isn't complete enough to convince myself that the value being passed in to UpdateDimensions is actually in ScreenPixels, and that it's reasonable to turn it into app units by using AppUnitsPerDevPixelAtUnitFullZoom. I looked at the GetWindowDimensions function that provides the rect that's passed to UpdateDimensions but I don't know what nsIBaseWindow actually represents and what coordinate system it's GetPosition/Size values are in. I'm attaching a picture that shows what my mental model is. Can you tell me what the UpdateDimensions rect would correspond to in this picture?

Flags: needinfo?(bugmail.mozilla)

Assignee

Comment 88

•

10 years ago

The rect in UpdateDimensions (currently) represents the widget's screen location and size, scaled by the display device-scale. The whole widget. So zoom isn't relevant to it. The value of GetWindowDimensions should therefore not be affected by any zoom (content, chrome, whatever). The PuppetWidget's position/size therefore exactly reflects the OS's widget. It just considers the browser chrome to be part of the 'non-client' region (which normally is the region where the OS puts window decorations). This stuff is key for things like Event::GetScreenCoordinates. So, on my retina MBP, the screen is 1440x900 in unscaled units and 2880x1800 in scaled units. UpdateDimensions's rect (in fullscreen) is therefore 2880x1800. The size parameter (poorly named) reflects what I've been referring to as the content area (aka "???" in your picture). Does that help?

Flags: needinfo?(bugmail.mozilla)

Comment 89

•

10 years ago

Thanks, that helps too. So I'm reasonably convinced now that there are issues with the code but they only occur in scenarios that we don't care about right now. Specifically if the root presshell on the TabParent side has a resolution set on it, I think there will be problems because that's when the LayoutDevicePixels don't equal the ScreenPixels. However as far as I know that's not something that will happen until either we allow pinch-zooming the entire browser chrome (unlikely) or nested content processes in B2G finally is enabled (which has been in the works but AFAIK is stalled right now). Also if the root presShell does have a resolution, then a lot of other stuff will break too so I don't think it's a problem if this is broken too, it won't even be noticeable until we clean up the mountain of other stuff. For example the cast from LayoutDevice to Screen pixels at [1] (which is where the size parameter appears to be coming from) probably needs to take into account the resolution too. Just one more question for now: At [2] when we add the client offset, don't we need to truncate the width/height of the rect by the same amount? Otherwise the rect will "leak" outside the widget area. The only remaining case that I'm slightly worried about is if the root presshell in the child process has a resolution set. So for example if you go into the B2G browser, load a page, and zoom in/out. Rotating the device should then also trigger UpdateDimensions calls, and if the screenX/screenY coordinates of events all come out correctly in those scenarios (or at least no differently than without the patch) then I'm fine with landing this. You can use my test page at http://people.mozilla.org/~kgupta/grid.html to check this (it will log touch events to logcat). Thanks for being so patient and helping me understand how this stuff works! [1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=a8963c3c2c56#289 [2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=a8963c3c2c56#951

Flags: needinfo?(bugmail.mozilla)

Assignee

•

10 years ago

> So for example if you > go into the B2G browser, load a page, and zoom in/out. Rotating the device > should then also trigger UpdateDimensions calls, and if the screenX/screenY > coordinates of events all come out correctly in those scenarios (or at least > no differently than without the patch) then I'm fine with landing this. You > can use my test page at http://people.mozilla.org/~kgupta/grid.html to check > this (it will log touch events to logcat). I was expecting the logged screen coordinates *not* to scale with the zoom but they did... then I tried without this patch and they show the same behavior (they line up with the grid, modulo the titlebar, regardless of zoom). The behavior was the same with various combinations of zooming and rotating. This was on Flame. I'll make the rest of kats' changes and ping roc to refresh the review.

Comment 94

•

10 years ago

Sounds good, thanks!

Assignee

Comment 95

•

Attachment #8604220 - Flags: review?(bugs)

•

10 years ago

Attachment #8604286 - Attachment description: 1. Make TabParent/TabChild UpdateDimensions messages aware of the display scale → 1. Store TabParents with their root window

Assignee

Comment 105

•

10 years ago

Retrying canceled tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc868fb51cfd

Comment 106

•

10 years ago

Comment on attachment 8604286 [details] [diff] [review] 1. Store TabParents with their root window Review of attachment 8604286 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ +1000,5 @@ > + } > + nsRect normalizedRect(mRect.x, mRect.y, mRect.width, mRect.height); > + normalizedRect.ScaleInverseRoundOut(widgetScale); > + nsSize normalizedSize(static_cast<double>(mDimensions.width) / widgetScale, > + static_cast<double>(mDimensions.height) / widgetScale); This is wrong. mRect and mDimensions are in ScreenPixels (which here are equivalent to LayoutDevicePixels) and widget->GetDefaultScale is a CSSToLayoutDeviceScale. So "normalizedRect" and "normalizedSize" are in CSSPixels and should not be stored in nsRect/nsSize (which are for app units only). Please rewrite this like so: CSSToLayoutDeviceScale widgetScale; if (widget) { widgetScale = widget->GetDefaultScale(); } CSSRect normalizedRect = ViewAs<LayoutDevicePixel>(mRect) / widgetScale; CSSSize normalizedSize = ViewAs<LayoutDevicePixel>(mDimensions) / widgetScale; I'm not a huge fan of "normalizedRect" and "normalizedSize" either but whatever. And shouldn't this hunk be in the part 2 patch?

Comment 107

•

10 years ago

Comment on attachment 8604288 [details] [diff] [review] 2. Make TabParent/TabChild UpdateDimensions messages aware of the display scale Review of attachment 8604288 [details] [diff] [review]: ----------------------------------------------------------------- The good news is that none of my suggested changes should actually change the mathematics of the values flowing around, they just updated variables to have the right types :) I'd like to see the updated patch with the changes so minusing for now. ::: dom/ipc/PBrowser.ipdl @@ +44,5 @@ > using class mozilla::WidgetMouseEvent from "ipc/nsGUIEventIPC.h"; > using class mozilla::WidgetWheelEvent from "ipc/nsGUIEventIPC.h"; > using class mozilla::WidgetDragEvent from "ipc/nsGUIEventIPC.h"; > using struct nsRect from "nsRect.h"; > +using struct nsSize from "nsSize.h"; Won't need this @@ +544,5 @@ > LoadURL(nsCString uri, BrowserConfiguration config); > > CacheFileDescriptor(nsString path, FileDescriptor fd); > > + UpdateDimensions(nsRect rect, nsSize size, ScreenOrientation orientation, Based on the changes in part 1, these are CSSRect and CSSSize ::: dom/ipc/TabChild.h @@ +320,5 @@ > const uint64_t& aLayersId, > PRenderFrameChild* aRenderFrame, > const bool& aParentIsActive) override; > + virtual bool RecvUpdateDimensions(const nsRect& rect, > + const nsSize& size, CSSRect, CSSSize @@ +502,5 @@ > } > > + virtual ScreenIntSize GetInnerSize() { > + float scale = mWidget->GetDefaultScale().scale; > + return ScreenIntSize(mNormalizedInnerSize.width * scale, mNormalizedInnerSize.height * scale); Same changes here as what I say for GetOuterRect below @@ +596,5 @@ > void SetTabId(const TabId& aTabId); > > + ScreenIntRect GetOuterRect() { > + nsIntRect rect = mNormalizedOuterRect.ToNearestPixels(mWidget->GetDefaultScale().scale); > + return ScreenIntRect::FromUnknownRect(rect); LayoutDeviceIntRect outerRect = RoundedToInt(mNormalizedOuterRect * mWidget->GetDefaultScale()); return ViewAs<ScreenPixel>(outerRect, PixelCastJustification::LayoutDeviceIsScreenForOuter); (You'll need to add the LayoutDeviceIsScreenForOuter value to the PixelCastJustfication enum) @@ +599,5 @@ > + nsIntRect rect = mNormalizedOuterRect.ToNearestPixels(mWidget->GetDefaultScale().scale); > + return ScreenIntRect::FromUnknownRect(rect); > + } > + > + void SetNormalizedInnerSize(const nsSize& size) { CSSSize @@ +615,5 @@ > RenderFrameChild* mRemoteFrame; > nsRefPtr<nsIContentChild> mManager; > uint32_t mChromeFlags; > uint64_t mLayersId; > + nsRect mNormalizedOuterRect; CSSRect @@ +649,5 @@ > float mDPI; > double mDefaultScale; > bool mIPCOpen; > bool mParentIsActive; > + nsSize mNormalizedInnerSize; CSSSize ::: gfx/ipc/GfxMessageUtils.h @@ +445,5 @@ > > +template<> > +struct ParamTraits<nsSize> > +{ > + typedef nsSize paramType; Won't need this any more

Attachment #8604288 - Flags: review?(bugmail.mozilla) → review-

Assignee

Comment 108

•

10 years ago

Attached patch Store TabParents with their root window (obsolete) — Details — Splinter Review

I haven't made kats' changes yet (they don't look controversial) -- I've been dealing with the orange that showed up in the tests. Good news is that the orange has been dealt with -- tests for these two patches are in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7bc7c7ccc9 Bad news is that I had to undo the change to nsContentUtils as the iteration was happening over some unwanted elements, causing new use-after-destroy actor bugs (not a use-after-free memory bug). Restricting the TabParent change that smaug proposed to the nsPresContext routine instead turns us back to green.

Attachment #8604286 - Attachment is obsolete: true

Attachment #8604288 - Attachment is obsolete: true

Attachment #8604286 - Flags: review?(bugs)

Assignee

Comment 109

•

10 years ago

Attached patch 2. Make TabParent/TabChild UpdateDimensions messages aware of the display scale (obsolete) — Details — Splinter Review

Second part of the patch

Assignee

Comment 110

•

10 years ago

Attached patch 1. Make TabParent/TabChild UpdateDimensions messages aware of the display scale (obsolete) — Details — Splinter Review

In addition to all of the basic changes: (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #106) > Comment on attachment 8604286 [details] [diff] [review] > 1. Store TabParents with their root window > > Review of attachment 8604286 [details] [diff] [review]: > ----------------------------------------------------------------- > Please rewrite this like so: > > CSSToLayoutDeviceScale widgetScale; > if (widget) { > widgetScale = widget->GetDefaultScale(); > } > CSSRect normalizedRect = ViewAs<LayoutDevicePixel>(mRect) / widgetScale; > CSSSize normalizedSize = ViewAs<LayoutDevicePixel>(mDimensions) / > widgetScale; I went a little wordier and used the PixelCastJustification I've switched the units. > And shouldn't this hunk be in the part 2 patch? Indeed. Fixed.

Attachment #8604690 - Attachment is obsolete: true

Attachment #8606572 - Flags: review?(bugmail.mozilla)

Assignee

Comment 111

•

10 years ago

Attached patch 2. Store TabParents with their root window (obsolete) — Details — Splinter Review

The comments from the old patch (comment 101) are still valid: > smaug, we had conjectured that the Tabs could attach to the top-level win on > construction and then we'd be done but we forgot about dragging tabs between > windows. Since dragging tabs changes the top-level window, I changed to > register/unregister when the Tab's nsIContent changes. Additionally, I had to add the line in TabParent::UIResolutionChanged for tests. I ran this past mconley, who wrote the original behavior, and he says it shouldn't break any contracts -- the only way this could create unexpected behavior is if we invalidate a tab's resolution for a widget change before changing the tab's widget, which would be odd. Tests (with both patches applied): https://treeherder.mozilla.org/#/jobs?repo=try&revision=586b940296f5

Attachment #8604689 - Attachment is obsolete: true

Attachment #8606574 - Flags: review?(bugs)

Comment 112

•

10 years ago

Comment on attachment 8606572 [details] [diff] [review] 1. Make TabParent/TabChild UpdateDimensions messages aware of the display scale Review of attachment 8606572 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: dom/ipc/TabChild.cpp @@ +860,5 @@ > , mRemoteFrame(nullptr) > , mManager(aManager) > , mChromeFlags(aChromeFlags) > , mLayersId(0) > + , mUnscaledOuterRect(0, 0, 0, 0) Technically not needed since CSSRect gets initialized to zeros anyway @@ +878,5 @@ > , mDPI(0) > , mDefaultScale(0) > , mIPCOpen(true) > , mParentIsActive(false) > + , mUnscaledInnerSize(0, 0) Ditto ::: dom/ipc/TabChild.h @@ +594,5 @@ > void SetTabId(const TabId& aTabId); > > + ScreenIntRect GetOuterRect(); > + > + void SetUnscaledInnerSize(const CSSSize& size) { s/size/aSize/

Attachment #8606572 - Flags: review?(bugmail.mozilla) → review+

Comment 113

•

10 years ago

(In reply to David Parks [:handyman] from comment #111) > 2. Store TabParents with their root window WindowRoot, not root window.

•

10 years ago

Comment on attachment 8611540 [details] [diff] [review] 2. Store TabParents with their WindowRoot >+already_AddRefed<nsPIWindowRoot> >+nsContentUtils::GetWindowRoot(nsIDocument* aDoc) >+{ >+ while (aDoc) { >+ nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow(); >+ if (win) { >+ return win->GetTopWindowRoot(); >+ } >+ aDoc = aDoc->GetParentDocument(); >+ } Actually, if (aDoc) { nsPIDOMWindow* win = aDoc->GetWindow(); if (win) { return win->GetTopWindowRoot(); } } return nullptr; should be enough. > #define NS_IWINDOWROOT_IID \ > { 0x728a2682, 0x55c0, 0x4860, \ > { 0x82, 0x6b, 0x0c, 0x30, 0x0a, 0xac, 0xaa, 0x60 } } Update this IID >+ typedef void (*BrowserEnumerator)(mozilla::dom::TabParent *aTab, void *aArg); >+ >+ // Enumerate all stored browsers that for which the weak reference is valid. >+ virtual void EnumerateBrowsers(BrowserEnumerator aEnumFunc, void *aArg) = 0; * goes with the type, not with argument name Same also elsewhere. >+static PLDHashOperator >+WeakBrowserEnumFunc(nsRefPtrHashKey<nsIWeakReference> *aKey, void *aArg) >+{ >+ WeakBrowserCallback* callback = static_cast<WeakBrowserCallback*>(aArg); >+ nsCOMPtr<nsITabParent> opener(do_QueryReferent((*aKey).GetKey())); opener? >+ TabParent* tab = TabParent::GetFrom(opener); >+ if (tab) { >+ callback->mUserFunc(tab, callback->mUserArg); >+ } >+ return PL_DHASH_NEXT; >+} >+ >+void >+nsWindowRoot::EnumerateBrowsers(BrowserEnumerator aEnumFunc, void *aArg) >+{ >+ WeakBrowserCallback callback; >+ callback.mUserFunc = aEnumFunc; >+ callback.mUserArg = aArg; >+ mWeakBrowsers.EnumerateEntries(WeakBrowserEnumFunc, &callback); I think this could would be a bit simpler and safer if you collected TabBrowsers to nsTArray<nsRefPtr<TabParent>> tabParents; using EnumerateEntries() static PLDHashOperator WeakBrowserEnumFunc(nsRefPtrHashKey<nsIWeakReference>* aKey, void* aArg) { nsTArray<nsRefPtr<TabParent>>* tabParents = static_cast<nsTArray<nsRefPtr<TabParent>>*>(aArgs); nsCOMPtr<nsITabParent> tabParent(do_QueryReferent((*aKey).GetKey())); if (tabParent) { tabParents->AppendElement(tabParent); } } nsTArray<nsRefPtr<TabParent>> tabParents; mWeakBrowsers.EnumerateEntries(WeakBrowserEnumFunc, &tabParent); and then just for (var i = 0; i < tabParents.Length(); ++i) { aEnumFunc(tabParents[i], aArg); } This way it would be ok for aEnumFunc to end up causing changes to mWeakBrowsers (for example to remove TabParent) You could then drop struct WeakBrowserCallback too. Sure, it is O(2n) vs O(n), but I don't think that is an issue. With those changes r+. (if you think the change to enumeration needs a new review, feel free to ask such)

Attachment #8611540 - Flags: review?(bugs) → review+

Liz Henry (:lizzard)

Comment 119

•

10 years ago

If this lands soon in m-c, let's verify the fix in nightly and uplift it to aurora. David when that's the case can you request upplift? Depending on how that goes we might also want to uplift to beta.

Flags: qe-verify+

Flags: needinfo?(florin.mezei)

Assignee

Comment 120

•

10 years ago

Attached patch 2. Store TabParents with their WindowRoot (r+ed by smaug) (obsolete) — Details — Splinter Review

Originally r+ed in comment 118. Includes smaug's requested changes. (In reply to Liz Henry (:lizzard) from comment #119) > If this lands soon in m-c, let's verify the fix in nightly and uplift it to > aurora. David when that's the case can you request upplift? Sure thing, Liz. (In reply to Olli Pettay [:smaug] from comment #118) > Comment on attachment 8611540 [details] [diff] [review] > 2. Store TabParents with their WindowRoot > > (if you think the change to enumeration needs a new review, feel free to ask > such) Eh, it was basically cutting and pasting from your review. I'm just going to check it in. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=321463ea63b4

Attachment #8611540 - Attachment is obsolete: true

Assignee

Updated

•

10 years ago

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 121

•

10 years ago

Hi, this patch failed to apply: patching file dom/ipc/TabChild.cpp Hunk #1 FAILED at 234 1 out of 2 hunks FAILED -- saving rejects to file dom/ipc/TabChild.cpp.rej could you take a look, thanks!

Flags: needinfo?(davidp99)

Keywords: checkin-needed

Assignee

Comment 122

•

10 years ago

Attached patch 1. Make TabParent/TabChild UpdateDimensions messages aware of the display scale (r+ed by kats) — Details — Splinter Review

de-bitrot and try again...

Attachment #8611533 - Attachment is obsolete: true

Flags: needinfo?(davidp99)

Assignee

Comment 123

•

10 years ago

Attached patch 2. Store TabParents with their WindowRoot (r+ed by smaug) — Details — Splinter Review

de-bitrot

Attachment #8614858 - Attachment is obsolete: true

Assignee

Updated

•

10 years ago

Keywords: checkin-needed

Pulsebot

Comment 124

•

Resolution: FIXED → ---

Comment 130

•

10 years ago

I can reproduce bug 1173224 on Aries with the patches reapplied on master. I'm investigating.

Comment 131

•

10 years ago

So the problem is that sometimes when SendUpdateDimensions is called on the TabParent, the GetWidget() returns null, and so we just use 1 for the widget scale. This ends up sending bad values over the pipe to the child. I later see the right values flowing through again, but it seems like the damage is done and the TabChild lays out the keyboard based on the bad values that came through earlier. So there might be a couple of bugs here; one is the fact that we're sending over bad values, and the other is that the child process doesn't relayout when the transiently-bad values are corrected to good values. I can write a simple patch to fix the first issue and assuming everything is green/it fixes this bug I think we should land it. I don't know how involved it will be to fix the second issue.

Comment 132

•

10 years ago

Attached patch 3. override fixes — Details — Splinter Review

Attachment #8621726 - Flags: review+

Comment 133

•

10 years ago

Attached patch 4. Fix for sending over bad data when widget is null — Details — Splinter Review

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c16bb06ad55f

Attachment #8621730 - Flags: review?(davidp99)

Attachment #8621730 - Flags: review?(bugs)

Comment 134

•

10 years ago

Comment on attachment 8621730 [details] [diff] [review] 4. Fix for sending over bad data when widget is null It is totally bizarre state to not have widget. Do you know when that happens? Sounds like a bug elsewhere. Perhaps add NS_WARNING to if (!widget) But I guess we can take this anyhow.

Attachment #8621730 - Flags: review?(bugs) → review+

Assignee

Comment 135

•

10 years ago

•

10 years ago

I was able to reliably reproduce the original issue on a MacBook Pro Retina, 15-inch, Late 2013, with an external monitor, using Nightly from 2015-05-25, and the following steps: 1. Have the external monitor disconnected from the MacBook. 2. Start Nightly and leave just the default about:home tab open. 3. Resize Nightly to Full Screen mode (use expand arrows in upper right corner of browser window or shift+command+f). 4. Connect the external monitor by plugging the HDMI cable into the external display port. 5. Once the external monitor is connected, close the MacBook's lid -> Nightly is moved to the external monitor and resizes incorrectly showing about 1/4 of the actual page. 6. Open the MacBook's lid again -> Nightly is moved back to the MacBook screen, but the page is still incorrectly resized. I was able to reproduce the above issue only with e10s enabled, and was able to verify the fix (with e10s enabled) on latest Nightly 41 (BuildID=20150618134629). I see the temporary resizing animation on various pages, but the pages are never stuck displaying at incorrect size. Since I am not able to reproduce in non-e10s mode, I'll keep the needinfo around to try again if I get some time, and I'll keep the bug Resolved to see if anyone else still sees this issue.

Kate Glazko

Comment 142

•

10 years ago

David, we are early in the Beta cycle. What is the status of this bug currently, and do you have any intention to uplift to Beta 40?

Flags: needinfo?(davidp99)

Botond Ballo [:botond]

•

10 years ago

I've tried again today to reproduce the issue using the latest Firefox 43 Nightly (e10s enabled) and Firefox 41 Beta 4, but I could not reproduce the issue. Since it's been more than a month without any regressions, or any additional reports of this happening, I'm closing it as verified.

Status: RESOLVED → VERIFIED

status-firefox41: fixed → verified

Flags: needinfo?(florin.mezei)

Reporter

Updated

•

10 years ago

Blocks: 1215659