445765 - window.screen sometimes maps to wrong screen in multiple monitor environment

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Sylvain Pasche]

Attachment: XUL testcase, should be opened toplevel with openDialog() for instance

The screen property fetched from a root window does not map to the current screen where the window is located, but to the main window. The property is correct if read from a child window. This means it is correct for content window, but not browser window (thus causing bug 445749).

In fact nsScreen::GetDeviceContext() returns a deviceContext that has no associated nsIWidget (mWidget is null), so nsThebesDeviceContext::FindScreen() uses GetPrimaryScreen() which is not correct when the window is on the right screen.

Comment 1

[Johnny Stenback (:jst)]

This sounds like a widget level bug to me. Reassigning.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch

In getting the device context from a docshell don't go through nsIBaseWindow::GetMainWidget because that always returns the device context for the primary screen when called on toplevel docshell.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 396094 [details] [diff] [review]
patch

I don't feel like much of an expert on this, but I have no idea who would, so r=dbaron.

Comment 4

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6b686281f9ac

Comment 5

[(no longer active)]

Could this have affected Txul?

Regression: Txul increase 3.19% on Linux Firefox

Previous results: 108.947 from build 20090822192711 of revision 0c55df2e3f99 at 2009-08-22 19:27:00 on talos-rev2-linux18 run # 0

New results: 112.421 from build 20090822232404 of revision 6b686281f9ac at 2009-08-22 23:24:00 on talos-rev2-linux01 run # 0

Suspected checkin range: from revision 0c55df2e3f99 to revision 6b686281f9ac

Checkin range only includes this bug.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The graphs were pretty clear, so I backed it out:

http://hg.mozilla.org/mozilla-central/rev/85899e12310f
http://hg.mozilla.org/mozilla-central/rev/e455909e2a17

I'm pretty surprised, though.  Maybe it changed something about initialization order because something gets window.screen early in window creation?


We'll see if the backout fixes the regression, at least...

Comment 7

[Timothy Nikkel (:tnikkel)]

I'm stumped for now. I've stepped through both the new and old code running Txul, nothing is created, nothing happens that would seem to take any more time, just a bunch of ref/com ptr and queryinterface stuff. There are 3 calls to GetDeviceContextForScreenInfo on a window creation, two come through nsXULWindow::OnChromeLoaded for availHeight and availWidth, one comes from js when onload fires wanting availWidth. On Linux, the actual screen properties are stored in the nsScreenGTK on Init and only changed via event callback as far as I can tell.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: bypass expensive system calls for one monitor case

Doing some timing on my debug build I found a culprit taking about 30ms (everything else was dwarfed by this). The device context we get via nsIBaseWidget::GetMainWidget doesn't have a widget, so any screen related calls to that device context default to the primary screen. But if we do have a widget (what the patch in this bug does) then we have to go through nsScreenManagerGtk::ScreenForNativeWidget which makes two system calls that can sometimes be expensive. These system calls are a waste of time on single monitor systems because we can only ever return the one monitor. So skip the system calls in the common one monitor case.

Could someone submit these two patches in this bug to the tryserver so we can determine if the Txul regression is solved?

Comment 9

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

OK, I just pushed it to tryserver. I'm not going to push anything else tonight so it will be the latest [e-mail] build as of tomorrow morning.

Comment 10

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

Oops, I did two pushes, one with just the first patch and one with both patches.

Comment 11

[Timothy Nikkel (:tnikkel)]

Thank you for Robert for sending it to the try server.

Because of the two pushes I'm having trouble identifying which Twinopen result has both patches incorporated. One has a bad Twinopen time of 115.37 and one has a good time of 108.61. I'm not sure how try works, I would imagine it clears all changes and go back to a clean m-c pull after every push. In that case it seems like Robert did two pushes, each with one patch?

Comment 12

[Timothy Nikkel (:tnikkel)]

Figured it out by looking at the files on the hg webserver for those revisions. The 108.61 is for both patches, 115.37 is for just the first patch. So the second patch does solve the Txul regression.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 396313 [details] [diff] [review]
bypass expensive system calls for one monitor case

r=dbaron.

But it would be good to get a bug filed on the multi-monitor case here too; taking a 3% new window time hit for multi-monitor isn't exactly ideal.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/b5f5c94eba5d
http://hg.mozilla.org/mozilla-central/rev/eacf5f1da49c

Don't forget to file that bug, though.

Comment 15

[Timothy Nikkel (:tnikkel)]

Thanks for landing those.

Bug 513985 filed on making nsScreenManagerGtk::ScreenForNativeWidget faster in the multi-monitor case. I would have filed it sooner (and added checkin-needed) but I wanted to think about the issue a little bit more.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0ee903f19661
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/871a0dcb933c

Comment 17

[Tanner M. Young [:tmyoung]]

Verified Fixed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090915 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090915052755