Closed Bug 1012487 Opened 9 years ago Closed 1 year ago

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

Categories

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

x86
Windows 8
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox31 --- unaffected
firefox32 - unaffected
firefox33 - affected
firefox34 --- affected

People

(Reporter: karlt, Assigned: bas.schouten)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

+++ 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.
Retriggers on the previous push to confirm the regressing push.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5bb98302ae5d
OS: Windows 7 → Windows 8
Hardware: x86_64 → x86
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.
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.
Assignee: nobody → bas
Status: NEW → ASSIGNED
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.
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.
Attachment #8424718 - Attachment is obsolete: true
Attachment #8424874 - Flags: review?(jmathies)
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 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.
Attachment #8424874 - Flags: review?(jmathies) → review+
Attachment #8424874 - Attachment is obsolete: true
Attachment #8424897 - Flags: review?(jmathies)
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.
Attachment #8424897 - Flags: review?(jmathies) → review+
This bug should stay open once this patch lands! I'll add a more involved patch here tomorrow.
Bug marking is an automated process, so please set the keyword in that case.
Keywords: leave-open
Are you thinking of this sort of thing?
Attachment #8425581 - Flags: feedback?
Attachment #8425581 - Flags: feedback? → feedback?(jmathies)
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.
Attachment #8425581 - Flags: feedback?(jmathies) → feedback+
(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 :-)
Can this bug be closed? Patches have landed and no activity for a while.
(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'.
Dropping tracking for 32 as OMTC was turned off in this version. Marking tracking for 33.
Bas, how can we make progress on this? We have time for 33 but not much. 33 beta 4 is going to build today.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
OK. Untracking it then. Thanks
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.