1012487 - intermittent random WINNT reftest-no-accel failures due to bad scale since OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #899785 +++

See for example spellcheck-slash-valid.html failure at
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2708630b4998
or menclose-2-phasorangle.html, block-horizontal-3-dyn.html at
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f6100b38ffd7

Note the scrollbar.

Comment 1

[Karl Tomlinson (:karlt)]

Retriggers on the previous push to confirm the regressing push.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5bb98302ae5d

Comment 2

[Karl Tomlinson (:karlt)]

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

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is interesting.

The presence of scroll bars would suggest that layout is aware of, and is drawing the increased scale. A bug purely in the compositor would just draw things larger, without including scrollbars.

Every time a test fails in this way, we get a MozAfterPaint event fired like this "[CONTENT] Rect: 0 0 14 17".

14x17 is the page size (800x1000) divided by 60 (number of app units per pixel) rounded up to the nearest integer. I guess that means we're confusing app units and pixel somewhere, but only with BasicCompositor enabled.

I can't think of any code right now that would be unique to OMTC+Basic which could affect layout.

Comment 4

[Bas Schouten (:bas.schouten)]

I think I've found something of interest, the documentation of ::GetDC(nullptr) says the handle to a DC can only be used by a single thread at the same time. I suspect we're running into contention and the ::GetDeviceCaps function that gets our DPI from mWidget->GetDefaultScale() occasionally returns 0 because of that. I suspect this causes nsDeviceContext.cpp:358 to incorrectly get 1.0f in mAppUnitsPerDevNotScaledPixel. This would cause the rendering artifacts we see, I've pushed a patch that forces devPixelsPerCSSPixel to 1.0f in order to confirm. If this is the cause I'll need to think about what the best solution is. A short term fix in that case would probably to do an if (DevPixelsPerCSSPixel == 0) { devPixelsPerCSSPixel = 1.0f; }. This will be acceptable for the nightly audience I believe.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Default to 100% DPI when GetDefaultScale fails.

Comment 6

[Bas Schouten (:bas.schouten)]

This theory has been confirmed (https://tbpl.mozilla.org/php/getParsedLog.php?id=39944049&tree=Try&full=1#error1), I'm working on a slightly different short-term fix.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Default to 100% DPI when GetDeviceCaps fails.

This patch should be acceptable for nightly, in the (evidently extremely rare) occasion where we have contention on the ScreenDC, this will default to 100% DPI for drawing that particular frame on that particular layer. This should fix our tests and offer an acceptable UX for nightly. There's several ways to fix this longer term, we'll discuss them later.

Comment 8

[Jim Mathies [:jimm]]

Isn't this going to cause all sorts of drawing anomalies?

I think a better fix would be to prime and cache the result in a static and just return that. (Removing repetitive GetDC/GetDeviceCaps calls can't hurt). You could reset this result to 0 and re-query after WM_DPICHANGED is sent on win8.

alternatively, couldn't we just use a lock here to be sure only one thread accesses the value at a time?

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8424874 [details] [diff] [review]
Default to 100% DPI when GetDeviceCaps fails.

<Bas> jimm: We don't know when cairo's internally using the DC so locking is not an option. The other thing I'm okay with, but it would cause significant behavior change, DPI settings but no longer be live but rather per startup. Also the rendering artifacts aren't that bad.. it just draws 'normal' for a single frame on -very- rare occasions. Which will only be bad for users with a custom DPI.
* bc is now known as bc|afk
<Bas> jimm: Sound acceptable? (Sorry, I have to go in 20 mins)
<jimm> Bas: not sure what you suggesting as your fix
<jimm> what you posted?
<jimm> hacks like that tend to become features in widget code. I'd really prefer you try to fix it right the first time.
<jimm> caching doesn't seem like a bad solution. I don't think you can change dpi on os <= win7 without a reboot 
<jimm> and win8.1 has a new event it sends when the dpi updates
<Bas> jimm: This fix is already done, and zero-risk. I want to land this first so we can re-land OMTC and still get the 3 weeks of testing I -really- want to get, the occurance is less than 1 in 20 000 page loads. I promise I'll create the other more complicated patch -before- the end of tomorrow :)
<jimm> Bas: alright, please file a follow up and request appropriate blocking flags so we don't forget about it.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Default to 100% DPI when GetDeviceCaps fails. v2

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 8424897 [details] [diff] [review]
Default to 100% DPI when GetDeviceCaps fails. v2

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

Carrying review, this just fixes a forgotten qref.

Comment 12

[Bas Schouten (:bas.schouten)]

This bug should stay open once this patch lands! I'll add a more involved patch here tomorrow.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Bug marking is an automated process, so please set the keyword in that case.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b999b33fe0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17b999b33fe0

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Only get DPI once

Are you thinking of this sort of thing?

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8425581 [details] [diff] [review]
Only get DPI once

yeah, exactly. I'm not sure how we currently deal with dpi changes though on win 8.1. You can flip dpi on the fly there. Not sure if firefox reacts to that or keeps its old settings until you restart.

The os sends an event that could reset sLogToPhysRetrieved to false, and then you could trigger an invalidate on all windows to handle that properly. The other thing 8.1 does is send the event when you drag between monitors with different dpi settings. Not sure if we're dealing with that at all, I don't see a handler for the event type.

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to Jim Mathies [:jimm] from comment #17)
> Comment on attachment 8425581 [details] [diff] [review]
> Only get DPI once
> 
> yeah, exactly. I'm not sure how we currently deal with dpi changes though on
> win 8.1. You can flip dpi on the fly there. Not sure if firefox reacts to
> that or keeps its old settings until you restart.
> 
> The os sends an event that could reset sLogToPhysRetrieved to false, and
> then you could trigger an invalidate on all windows to handle that properly.
> The other thing 8.1 does is send the event when you drag between monitors
> with different dpi settings. Not sure if we're dealing with that at all, I
> don't see a handler for the event type.

I wouldn't be surprised if we just have artifacts :-)

Comment 19

[Nicolas Silva [:nical]]

Can this bug be closed? Patches have landed and no activity for a while.

Comment 20

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #19)
> Can this bug be closed? Patches have landed and no activity for a while.

In theory we still want a better fix, we have a proposal on here but I believe some people wanted something 'better'.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Dropping tracking for 32 as OMTC was turned off in this version. Marking tracking for 33.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Bas, how can we make progress on this? We have time for 33 but not much. 33 beta 4 is going to build today.

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Bas, how can we make progress on this? We have time for 33 but not much. 33
> beta 4 is going to build today.

This bug was fixed. We just want a nicer fix but 32 already shipped without that nicer fix. So we're not going to regress anything by not doing a nicer fix in 33. I don't think we should even be thinking about uplifting to Beta. I wish we had time to do the nicer fix but other 'higher priority' things keep getting in the way for all of us. We don't need to track this.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

OK. Untracking it then. Thanks

Comment 25

[BMO Automation]

Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.