Closed
Bug 1012487
Opened 10 years ago
Closed 2 years ago
intermittent random WINNT reftest-no-accel failures due to bad scale since OMTC
Categories
(Core :: Graphics: Layers, defect, P3)
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)
878 bytes,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
jimm
:
feedback+
|
Details | Diff | Splinter Review |
+++ 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.
Reporter | ||
Comment 1•10 years ago
|
||
Retriggers on the previous push to confirm the regressing push. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5bb98302ae5d
Reporter | ||
Updated•10 years ago
|
OS: Windows 7 → Windows 8
Hardware: x86_64 → x86
Reporter | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/php/getParsedLog.php?id=39914893&tree=Mozilla-Inbound
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Comment 5•10 years ago
|
||
Assignee: nobody → bas
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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)
![]() |
||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8424874 -
Attachment is obsolete: true
Attachment #8424897 -
Flags: review?(jmathies)
Assignee | ||
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
This bug should stay open once this patch lands! I'll add a more involved patch here tomorrow.
Comment 13•10 years ago
|
||
Bug marking is an automated process, so please set the keyword in that case.
Keywords: leave-open
![]() |
||
Updated•10 years ago
|
status-firefox32:
--- → affected
tracking-firefox32:
--- → ?
Assignee | ||
Comment 16•10 years ago
|
||
Are you thinking of this sort of thing?
Attachment #8425581 -
Flags: feedback?
Updated•10 years ago
|
Attachment #8425581 -
Flags: feedback? → feedback?(jmathies)
![]() |
||
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
(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 :-)
Updated•9 years ago
|
Comment 19•9 years ago
|
||
Can this bug be closed? Patches have landed and no activity for a while.
Assignee | ||
Comment 20•9 years ago
|
||
(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•9 years ago
|
||
Dropping tracking for 32 as OMTC was turned off in this version. Marking tracking for 33.
status-firefox31:
--- → unaffected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
tracking-firefox33:
--- → +
Comment 22•9 years ago
|
||
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)
Assignee | ||
Comment 23•9 years ago
|
||
(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)
Comment 25•7 years ago
|
||
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Assignee | ||
Updated•2 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•