Closed
Bug 1135315
Opened 9 years ago
Closed 8 years ago
The webide tests are going to permafail on OSX 10.8/10.10 debug when Gecko 38 merges to Aurora
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(firefox38+ wontfix, firefox39+ wontfix, firefox40 wontfix, firefox41 disabled, firefox42 disabled)
People
(Reporter: RyanVM, Unassigned)
Details
(Keywords: assertion)
Attachments
(2 files)
721 bytes,
patch
|
Details | Diff | Splinter Review | |
1.22 KB,
patch
|
smichaud
:
review-
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: Test permafail when Gecko 38 merges to Aurora on Monday. This only reproduces on OSX 10.8 debug, but I've reproduced it on 4 different pushes now, consistently. https://treeherder.mozilla.org/logviewer.html#?job_id=5076476&repo=try 15:36:46 INFO - 43 INFO TEST-START | browser/devtools/webide/test/test_addons.html 15:36:46 INFO - [1346] ###!!! ASSERTION: reflow state computed incorrect inline size: 'reflowState.ComputedISize() == size.ISize(wm) - reflowState.ComputedLogicalBorderPadding().IStartEnd(wm)', file /builds/slave/try-osx64-d-000000000000000000/build/src/layout/base/nsPresShell.cpp, line 9357 15:37:06 INFO - #01: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9524] 15:37:06 INFO - #02: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #03: PresShell::DidDoReflow(bool, bool) [layout/base/nsPresShell.cpp:9166] 15:37:06 INFO - #04: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9540] 15:37:06 INFO - #05: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #06: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [layout/base/nsIPresShell.h:296] 15:37:06 INFO - #07: mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp) [xpcom/glue/nsTArray.h:942] 15:37:06 INFO - #08: nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:647] 15:37:06 INFO - #09: nsTimerEvent::Run() [xpcom/base/nsRefPtr.h:44] 15:37:06 INFO - #10: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:855] 15:37:06 INFO - #11: NS_ProcessPendingEvents(nsIThread*, unsigned int) [xpcom/glue/nsThreadUtils.cpp:207] 15:37:06 INFO - #12: nsBaseAppShell::NativeEventCallback() [widget/nsBaseAppShell.cpp:99] 15:37:06 INFO - #13: nsAppShell::ProcessGeckoEvents(void*) [widget/cocoa/nsAppShell.mm:378] 15:37:06 INFO - #14: CoreFoundation + 0x12841 15:37:06 INFO - #15: CoreFoundation + 0x12165 15:37:06 INFO - #16: CoreFoundation + 0x354e5 15:37:06 INFO - #17: CoreFoundation + 0x34dd2 15:37:06 INFO - #18: HIToolbox + 0x5f774 15:37:06 INFO - #19: HIToolbox + 0x5f512 15:37:06 INFO - #20: HIToolbox + 0x5f3a3 15:37:06 INFO - #21: AppKit + 0x156fa3 15:37:06 INFO - #22: AppKit + 0x156862 15:37:06 INFO - #23: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [widget/cocoa/nsAppShell.mm:119] 15:37:06 INFO - #24: AppKit + 0x14dc03 15:37:06 INFO - #25: nsAppShell::Run() [xpcom/glue/nsCOMPtr.h:512] 15:37:06 INFO - #26: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:281] 15:37:06 INFO - #27: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4160] 15:37:06 INFO - #28: XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4236] 15:37:06 INFO - #29: XRE_main [toolkit/xre/nsAppRunner.cpp:4456] 15:37:06 INFO - #30: main [browser/app/nsBrowserApp.cpp:294] 15:37:06 INFO - [1346] ###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: '(target == rootFrame && size.BSize(wm) == NS_UNCONSTRAINEDSIZE) || (desiredSize.ISize(wm) == size.ISize(wm) && desiredSize.BSize(wm) == size.BSize(wm))', file /builds/slave/try-osx64-d-000000000000000000/build/src/layout/base/nsPresShell.cpp, line 9376 15:37:06 INFO - #01: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9524] 15:37:06 INFO - #02: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #03: PresShell::DidDoReflow(bool, bool) [layout/base/nsPresShell.cpp:9166] 15:37:06 INFO - #04: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9540] 15:37:06 INFO - #05: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #06: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [layout/base/nsIPresShell.h:296] 15:37:06 INFO - #07: mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp) [xpcom/glue/nsTArray.h:942] 15:37:06 INFO - #08: nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:647] 15:37:06 INFO - #09: nsTimerEvent::Run() [xpcom/base/nsRefPtr.h:44] 15:37:06 INFO - #10: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:855] 15:37:06 INFO - #11: NS_ProcessPendingEvents(nsIThread*, unsigned int) [xpcom/glue/nsThreadUtils.cpp:207] 15:37:06 INFO - #12: nsBaseAppShell::NativeEventCallback() [widget/nsBaseAppShell.cpp:99] 15:37:06 INFO - #13: nsAppShell::ProcessGeckoEvents(void*) [widget/cocoa/nsAppShell.mm:378] 15:37:06 INFO - #14: CoreFoundation + 0x12841 15:37:06 INFO - #15: CoreFoundation + 0x12165 15:37:06 INFO - #16: CoreFoundation + 0x354e5 15:37:06 INFO - #17: CoreFoundation + 0x34dd2 15:37:06 INFO - #18: HIToolbox + 0x5f774 15:37:06 INFO - #19: HIToolbox + 0x5f512 15:37:06 INFO - #20: HIToolbox + 0x5f3a3 15:37:06 INFO - #21: AppKit + 0x156fa3 15:37:06 INFO - #22: AppKit + 0x156862 15:37:06 INFO - #23: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [widget/cocoa/nsAppShell.mm:119] 15:37:06 INFO - #24: AppKit + 0x14dc03 15:37:06 INFO - #25: nsAppShell::Run() [xpcom/glue/nsCOMPtr.h:512] 15:37:06 INFO - #26: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:281] 15:37:06 INFO - #27: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4160] 15:37:06 INFO - #28: XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4236] 15:37:06 INFO - #29: XRE_main [toolkit/xre/nsAppRunner.cpp:4456] 15:37:06 INFO - #30: main [browser/app/nsBrowserApp.cpp:294] 15:37:06 INFO - [1346] ###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: '(target == rootFrame && size.BSize(wm) == NS_UNCONSTRAINEDSIZE) || (desiredSize.ISize(wm) == size.ISize(wm) && desiredSize.BSize(wm) == size.BSize(wm))', file /builds/slave/try-osx64-d-000000000000000000/build/src/layout/base/nsPresShell.cpp, line 9376 15:37:06 INFO - #01: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9524] 15:37:06 INFO - #02: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #03: PresShell::DidDoReflow(bool, bool) [layout/base/nsPresShell.cpp:9166] 15:37:06 INFO - #04: PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:9540] 15:37:06 INFO - #05: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4355] 15:37:06 INFO - #06: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [layout/base/nsIPresShell.h:296] 15:37:06 INFO - #07: mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp) [xpcom/glue/nsTArray.h:942] 15:37:06 INFO - #08: nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:647] 15:37:06 INFO - #09: nsTimerEvent::Run() [xpcom/base/nsRefPtr.h:44] 15:37:06 INFO - #10: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:855] 15:37:06 INFO - #11: NS_ProcessPendingEvents(nsIThread*, unsigned int) [xpcom/glue/nsThreadUtils.cpp:207] 15:37:06 INFO - #12: nsBaseAppShell::NativeEventCallback() [widget/nsBaseAppShell.cpp:99] 15:37:06 INFO - #13: nsAppShell::ProcessGeckoEvents(void*) [widget/cocoa/nsAppShell.mm:378] 15:37:06 INFO - #14: CoreFoundation + 0x12841 15:37:06 INFO - #15: CoreFoundation + 0x12165 15:37:06 INFO - #16: CoreFoundation + 0x354e5 15:37:06 INFO - #17: CoreFoundation + 0x34dd2 15:37:06 INFO - #18: HIToolbox + 0x5f774 15:37:06 INFO - #19: HIToolbox + 0x5f512 15:37:06 INFO - #20: HIToolbox + 0x5f3a3 15:37:06 INFO - #21: AppKit + 0x156fa3 15:37:06 INFO - #22: AppKit + 0x156862 15:37:06 INFO - #23: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [widget/cocoa/nsAppShell.mm:119] 15:37:06 INFO - #24: AppKit + 0x14dc03 15:37:06 INFO - #25: nsAppShell::Run() [xpcom/glue/nsCOMPtr.h:512] 15:37:06 INFO - #26: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:281] 15:37:06 INFO - #27: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4160] 15:37:06 INFO - #28: XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4236] 15:37:06 INFO - #29: XRE_main [toolkit/xre/nsAppRunner.cpp:4456] 15:37:06 INFO - #30: main [browser/app/nsBrowserApp.cpp:294] 15:37:06 INFO - 44 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | Helper has been auto-installed 15:37:06 INFO - 45 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | 3 simulator addons to install 15:37:06 INFO - 46 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | 5 addons listed 15:37:06 INFO - 47 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | 3 addons installed 15:37:06 INFO - 48 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | 2 addons uninstalled 15:37:06 INFO - 49 INFO Uninstalling Simulator 2.0 15:37:06 INFO - 50 INFO Uninstalling Simulator 3.0 15:37:06 INFO - 51 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | Found one runtime button 15:37:06 INFO - 52 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | Found 3 simulators button 15:37:06 INFO - 53 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | No simulator listed 15:37:06 INFO - 54 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | Warning about missing ADB hidden 15:37:06 INFO - 55 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | No usb runtime listed 15:37:06 INFO - 56 INFO TEST-PASS | browser/devtools/webide/test/test_addons.html | Warning about missing ADB present 15:37:06 INFO - 57 INFO Closing WebIDE 15:37:06 INFO - 58 INFO WebIDE closed 15:37:06 INFO - --DOCSHELL 0x10f575000 == 18 [pid = 1346] [id = 18] 15:37:06 INFO - --DOCSHELL 0x10f575800 == 17 [pid = 1346] [id = 19] 15:37:06 INFO - --DOCSHELL 0x10f576800 == 16 [pid = 1346] [id = 21] 15:37:06 INFO - --DOCSHELL 0x10f577000 == 15 [pid = 1346] [id = 22] 15:37:06 INFO - --DOCSHELL 0x10f577800 == 14 [pid = 1346] [id = 23] 15:37:06 INFO - --DOCSHELL 0x10f578000 == 13 [pid = 1346] [id = 24] 15:37:06 INFO - --DOCSHELL 0x10f578800 == 12 [pid = 1346] [id = 25] 15:37:06 INFO - --DOCSHELL 0x10f579000 == 11 [pid = 1346] [id = 26] 15:37:06 INFO - --DOCSHELL 0x10f579800 == 10 [pid = 1346] [id = 27] 15:37:06 INFO - [1346] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/slave/try-osx64-d-000000000000000000/build/src/dom/events/ContentEventHandler.cpp, line 110 15:37:06 INFO - 59 INFO TEST-OK | browser/devtools/webide/test/test_addons.html | took 2696ms 15:37:06 INFO - ++DOMWINDOW == 62 (0x1120fc800) [pid = 1346] [serial = 106] [outer = 0x124ed8000] 15:37:06 INFO - 60 INFO TEST-UNEXPECTED-ERROR | browser/devtools/webide/test/test_addons.html | Assertion count 3 is greater than expected range 0-0 assertions. Here's a raw diff of the patch used in the Try push. https://hg.mozilla.org/try/raw-rev/9f5d5a695b4f
Flags: needinfo?(paul)
Reporter | ||
Comment 2•9 years ago
|
||
This is a targeted patch to only disable them on 10.8 debug. Given the short notice of this issue, I'm OK with waiting until after the uplift and landing this directly on Aurora rather than forcing the issue on trunk. I've verified that this makes m-oth green on Try. Below is a link to the patch I used for the Try push. It applies cleanly to m-c. Otherwise, it should be directly reproducible after the Gecko 38 merge to Aurora tomorrow.
Reporter | ||
Comment 3•9 years ago
|
||
*sigh* http://people.mozilla.org/~rvandermeulen/trunk-as-aurora
Reporter | ||
Comment 4•9 years ago
|
||
The assertions hit post-merge as expected. Pushed the disabling patch to Aurora. https://hg.mozilla.org/releases/mozilla-aurora/rev/e685304832ad
I'll look into this soon. AIUI, this is no longer urgent since test is disabled now on Aurora.
Flags: needinfo?(paul)
Flags: needinfo?(jryans)
Ryan, I ran a try from aurora and removed the disable without any other changes, and things seem okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=916cfd8e16be Can we just remove the disable then? Or maybe I am missing something...
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 7•9 years ago
|
||
10.8 tests have to be specifically opted-in on Try. You only ran 10.6.
Flags: needinfo?(ryanvm)
Comment 8•9 years ago
|
||
Let's track this for 39. Is there someone who it should be assigned to it now so we can fix the tests before 39 moves up? For 38, do you plan to leave them disabled or try to fix them and uplift the fix? jryans, how crucial is it or what do you think the risk is for keeping these tests disabled in 38?
Flags: needinfo?(jryans)
Updated•9 years ago
|
Alex, could you investigate this? I'll be out next week.
Flags: needinfo?(jryans) → needinfo?(poirot.alex)
Comment 10•9 years ago
|
||
David, we are hitting an assertion introduced by Eli and r+ by you. I was wondering if that would interest you to know such assertion is hit when opening WebIDE in debug mode ? I don't see what we could do wrong in webide codebase... WebIDE is a xul document, opened in its own top level window: chrome://webide/content/ It is made of many iframes loading other xhtml chrome documents. The assertion is: ASSERTION: reflow state computed incorrect inline size: 'reflowState.ComputedISize() == size.ISize(wm) - reflowState.ComputedLogicalBorderPadding().IStartEnd(wm)' http://mxr.mozilla.org/mozilla-aurora/source/layout/base/nsPresShell.cpp#9354
Flags: needinfo?(poirot.alex) → needinfo?(dbaron)
What type of frame (i.e., what concrete class) is target? Is it the root frame (i.e., is its mParent null)? What are the values in question (and, perhaps most relevant, is reflowState.ComputedLogicalBorderPadding().IStartEnd(wm) nonzero)? Is target->IsThemed() true? And why would anything be different on aurora from central? I didn't think there were feature-enabling differences. Does it have something to do with developer edition?
Flags: needinfo?(dbaron)
Comment 12•9 years ago
|
||
Note that it only fail on osx 10.8. Yes, it looks like aurora is now built as Fx dev edition by default. I tried to run some try builds to see if that's related to fx dev edition, but so far I can't conclude anything. Build central with fx dev edition flags: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4606f97fdc Green Build aurora without fx dev edition flags: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3100d50a9e21 Green I'll try to run another try on aurora by only enabling the failing test, if that happens to be failing, it means that it is related to fx dev edition, but only on 10.8 aurora :x
Comment 13•9 years ago
|
||
Build aurora with just the test re-enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fa0bda374a5
Comment 14•9 years ago
|
||
Humm all these try are green, I'm not sure these tests are run in mochitest-devtools, but mochitest-chrome :s
Reporter | ||
Comment 15•9 years ago
|
||
Yep, mochitest-other is the suite that matters here.
Comment 16•9 years ago
|
||
Also fails on 10.10 aurora: https://treeherder.mozilla.org/logviewer.html#?job_id=687556&repo=mozilla-aurora
Reporter | ||
Comment 17•9 years ago
|
||
10.10 tests were enabled on Aurora yesterday and these failures persist. https://hg.mozilla.org/releases/mozilla-aurora/rev/fc83730483c7
Summary: The webide tests are going to permafail on OSX 10.8 debug when Gecko 38 merges to Aurora → The webide tests are going to permafail on OSX 10.8/10.10 debug when Gecko 38 merges to Aurora
Reporter | ||
Comment 18•9 years ago
|
||
On the bright side, I've confirmed that I can safely re-enable them on 38 after the next uplift. Whatever's going on here is Aurora (DevEdition?) only.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18) > On the bright side, I've confirmed that I can safely re-enable them on 38 > after the next uplift. Whatever's going on here is Aurora (DevEdition?) only. Do you mean the Dev Edition continues to be affected, as in Dev Edition 39 will have the same issue? Or do you mean the entire problem vanishes once 38 leaves Dev Edition?
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 20•9 years ago
|
||
It only affects the version on Aurora at any given time.
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 21•9 years ago
|
||
Disabled on Aurora39: https://hg.mozilla.org/releases/mozilla-aurora/rev/70b99015d154 Re-enabled on Beta38: https://hg.mozilla.org/releases/mozilla-beta/rev/5ab526e88a5b
I did a run with two new tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=912b93bc403e One opens WebIDE, one does not. Opening WebIDE triggers the assertion. My guess is it's caused by some of the DevTools common CSS we recently started importing, which then pulls in some theme things. Will need to dig into the styles used a bit further.
The error goes away if I remove theme-switching.js from the WebIDE logs panel: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67fce24ef24c This file loads a stylesheet based on the devtools.theme pref, which is "dark"[1] in Dev. Edition, and "light"[2] elsewhere. So, somehow the dark style sheet triggers this assertion but the light one does not. [1]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css [2]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #11) > What type of frame (i.e., what concrete class) is target? Is it the root > frame (i.e., is its mParent null)? What are the values in question (and, > perhaps most relevant, is > reflowState.ComputedLogicalBorderPadding().IStartEnd(wm) nonzero)? Is > target->IsThemed() true? I am not great at C++... Can I log target's concrete class name to the test output somehow? I can't reproduce this issue locally, so I have to send each attempt through try and review the log. > And why would anything be different on aurora from central? I didn't think > there were feature-enabling differences. Does it have something to do with > developer edition? So far, I've narrowed the issue down to the following: 1. WebIDE has a main XUL page (webide.xul)[1] with many XHTML iframes 2. One such iframe (logs.xhtml)[2] loads a DevTools theme switching script[3] 3. Part of the script[4] installs a floating scrollbar stylesheet[5][6] * This *only* happens when the devtools.theme is "dark", which is only true on Dev Edition If I remove the block that installs this scrollbar stylesheet, the assertions go away. Does this information give you any clues? Otherwise, any help on how best to log the info you asked for would be great! [1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/webide.xul [2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/logs.xhtml [3]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js [4]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#57 [5]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/floating-scrollbars-light.css [6]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/floating-scrollbars.css
Flags: needinfo?(dbaron)
Maybe Jonathan can help look into this; I've been pretty swamped the past few weeks.
Flags: needinfo?(dbaron) → needinfo?(jfkthame)
Comment 26•9 years ago
|
||
At the point when we hit the first assertion, size.ISize is 120 (and size.BSize is 480); IsThemed() is false; and reflowState.ComputedLogicalBorderPadding().IStartEnd(wm) is 240. It looks to me like the assertion occurs when we're trying to reflow the scrollbar with a width of only 2px, but the padding on the scrollbar means it wants at least 4px. The immediate culprit in the dark theme is the 2px padding used for the floating scrollbars: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/floating-scrollbars.css#10 This results in the 4px of ComputedBorderPadding() in the reflowState; but its ComputedISize(), which in theory should be -2px (the available width is 2px, and we lose 4px to the padding), gets clamped to zero in nsHTMLReflowState::InitConstraints. At the time this happens, it looks like the iframe has a size of 10px * 10px, and because we're using floating scrollbars, they don't take away any size from the scrollPort, so that's also 10px * 10px. The scrollbars have a width (or height, in the case of the horizontal scrollbar) of 8px, defined by floating-scrollbars.css, and this means that AdjustOverlappingScrollbars() in nsGfxScrollFrame.cpp will shorten them to a length of only 2px. And then their padding doesn't fit. Reducing the "padding:2px" on the floating scrollbars to "padding:1px" will make the assertions go away, but of course it also changes their appearance, which is presumably not desired. Maybe AdjustOverlappingScrollbars() should do something a bit different for floating scrollbars; perhaps it would be better to allow them to overlap than to shorten them below the minimum length implied by their BorderPadding? Hmm, looks like we even have support for that, in principle, via the nsLookAndFeel constant eIntID_AllowOverlayScrollbarsOverlap. But the Cocoa implementation only returns true if eIntID_UseOverlayScrollbars returns true, which in turn depends on the scrollbar setting in System Preferences / General. So if I change this from "Show scroll bars: Always" to "When scrolling", which makes the system use overlay scrollbars by default, the assertions go away.
Flags: needinfo?(jfkthame)
Comment 28•9 years ago
|
||
Because WebIDE uses overlay scrollbars in certain cases even when they're not specified by the system-level pref setting, I think we should remove the first check here so that the overlap behavior is used more consistently for these scrollbars on 10.8 and later.
Attachment #8594303 -
Flags: review?(smichaud)
Updated•9 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment 29•9 years ago
|
||
Comment on attachment 8594303 [details] [diff] [review] Allow floating scrollbars to overlap on OS X 10.8 and later, regardless of the system scrollbar preference I don't fully understand the implications of this patch, so I'm going to need to test it. We *don't* want to prevent people from turning off overlay scrollbars on OS X 10.8 and up.
Comment 30•9 years ago
|
||
(In reply to Steven Michaud from comment #29) > Comment on attachment 8594303 [details] [diff] [review] > Allow floating scrollbars to overlap on OS X 10.8 and later, regardless of > the system scrollbar preference > > I don't fully understand the implications of this patch, so I'm going to > need to test it. > > We *don't* want to prevent people from turning off overlay scrollbars on OS > X 10.8 and up. Right; indeed, I have them turned off on my 10.10 system, for example. (But WebIDE overrides that choice in at least one place via its CSS, as noted in comment 24.) AFAICT, this patch does not alter *whether* or *when* overlay scrollbars are used; all it does is to ensure that "forced" overlay scrollbars (i.e. ones that are explicitly enabled in CSS, even when the system preference didn't select them) get the same overlapping behavior as system-pref ones.
Comment 31•9 years ago
|
||
Comment on attachment 8594303 [details] [diff] [review] Allow floating scrollbars to overlap on OS X 10.8 and later, regardless of the system scrollbar preference I tested your patch and found that it doesn't make any harmful changes for those who, on OS X 10.8 and up, don't have overlay scrollbars enabled (because they've disabled them or have a mouse connected). But (as best I can tell) this is by accident, and I think we need to come up with another way to fix this bug. I just grepped the tree, and here is the only place (in current code) where this patch makes a difference: https://hg.mozilla.org/mozilla-central/annotate/17aad8f83237/layout/generic/nsGfxScrollFrame.cpp#l4835 So this patch *could* make non-overlay scrollbars overlap in the bottom right-hand corner -- even though it doesn't in current code. Somehow (in current code) non-overlay scrollbars are already properly sized (not to overlap) by the time this code is reached. It'd be much better to change what nsLookAndFeel::GetIntImpl() returns when it's called with aID == eIntID_UseOverlayScrollbars. Right now it returns the result of SystemWantsOverlayScrollbars(). But it should also take account of whether we've overridden the system setting (as in effect we do in WebIDE). I frankly don't know how to do this. But until we can come up with a good way, we should keep these tests disabled.
Attachment #8594303 -
Flags: review?(smichaud) → review-
Comment 32•9 years ago
|
||
(In reply to Steven Michaud from comment #31) > Comment on attachment 8594303 [details] [diff] [review] > Allow floating scrollbars to overlap on OS X 10.8 and later, regardless of > the system scrollbar preference > > I tested your patch and found that it doesn't make any harmful changes for > those who, on OS X 10.8 and up, don't have overlay scrollbars enabled > (because they've disabled them or have a mouse connected). But (as best I > can tell) this is by accident, and I think we need to come up with another > way to fix this bug. Non-overlay scrollbars don't overlap because they're positioned outside the scrollable viewport. As such, they will naturally leave a vacant square at the corner where they meet. (Potentially filled by a growbox, if present.) (If they were somehow styled to move *inside* the rect of the scrollable viewport, there'd be the possibility of overlap, but we don't do that -- except in the case of these WebIDE scrollbars, which are styled as overlays for which overlapping in the corner is fine.) > > I just grepped the tree, and here is the only place (in current code) where > this patch makes a difference: > > https://hg.mozilla.org/mozilla-central/annotate/17aad8f83237/layout/generic/ > nsGfxScrollFrame.cpp#l4835 > > So this patch *could* make non-overlay scrollbars overlap in the bottom > right-hand corner -- even though it doesn't in current code. Somehow (in > current code) non-overlay scrollbars are already properly sized (not to > overlap) by the time this code is reached. > > It'd be much better to change what nsLookAndFeel::GetIntImpl() returns when > it's called with aID == eIntID_UseOverlayScrollbars. Right now it returns > the result of SystemWantsOverlayScrollbars(). But it should also take > account of whether we've overridden the system setting (as in effect we do > in WebIDE). > > I frankly don't know how to do this. But until we can come up with a good > way, we should keep these tests disabled. I don't see how to do this either; the WebIDE scrollbars aren't "marked" in some way as being overlays, they're just styled with CSS that results in an overlay-scrollbar-like appearance, and moves them inside the scrollable port. So I still think this patch is a reasonable solution. But if we're not going to take it, then I'm out of ideas here for now.
Assignee: jfkthame → nobody
Status: ASSIGNED → NEW
Comment 33•9 years ago
|
||
> they're just styled with CSS that results in an
> overlay-scrollbar-like appearance, and moves them inside the
> scrollable port.
Could you tell me where in tree the code lives that does this?
Comment 34•9 years ago
|
||
(In reply to Steven Michaud from comment #33) > > they're just styled with CSS that results in an > > overlay-scrollbar-like appearance, and moves them inside the > > scrollable port. > > Could you tell me where in tree the code lives that does this? See comment 24: > [6]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/floating-scrollbars.css
Comment 35•9 years ago
|
||
(In reply to comment #34) I'm actually looking for the C++ code (presumably somewhere in layout) that "moves them inside the scrollable port".
Comment 36•9 years ago
|
||
(In reply to Steven Michaud from comment #35) > (In reply to comment #34) > > I'm actually looking for the C++ code (presumably somewhere in layout) that > "moves them inside the scrollable port". Isn't that what the negative margins in the CSS do?
Comment 37•9 years ago
|
||
I'm looking for C++ code, presumably somewhere in layout, to understand exactly how, under the hood, the CSS stuff is implemented.
Comment 38•9 years ago
|
||
(In reply to Steven Michaud from comment #37) > I'm looking for C++ code, presumably somewhere in layout, to understand > exactly how, under the hood, the CSS stuff is implemented. It sounds like you're looking for ScrollFrameHelper::LayoutScrollbars, in layout/generic/nsGfxScrollFrame.cpp, which positions the scroll bars (including applying margins if present).
Comment 39•9 years ago
|
||
Thanks! In the next few days (or more likely next week) I'll look at that code, and other code in the tree, to 1) understand better how overlay and "normal" scrollbars work, particularly in the case of CSS overrides, and 2) see what alternatives I can come up with to your patch.
Reporter | ||
Comment 40•9 years ago
|
||
Disabled on Aurora40: https://hg.mozilla.org/releases/mozilla-aurora/rev/1979f5036aab Re-enabled on Beta39: https://hg.mozilla.org/releases/mozilla-beta/rev/07d301081a0f
Reporter | ||
Comment 41•9 years ago
|
||
(In reply to Steven Michaud [:smichaud] from comment #39) > Thanks! > > In the next few days (or more likely next week) I'll look at that code, and > other code in the tree, to 1) understand better how overlay and "normal" > scrollbars work, particularly in the case of CSS overrides, and 2) see what > alternatives I can come up with to your patch. Any news here? :)
Flags: needinfo?(smichaud)
If there's no update, my vote is we should just disable the tests for Mac debug for all channels. The failure here is not specific to the test scenarios, but instead the product under test happens to be a situation that exercises this path (on DE channel only).
Comment 43•9 years ago
|
||
(In reply to comment #41) > Any news here? :) Nope, sorry :-( I wasn't able to get to this when I hoped, and I'm now swamped by other, much more urgent things. It will be at least a few weeks before I can back to this. In the meantime these tests should remain/be disabled.
Flags: needinfo?(smichaud)
Reporter | ||
Comment 44•9 years ago
|
||
Disabled on trunk. https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f55611971a
Whiteboard: [test disabled on OSX 10.10 debug][leave open]
Reporter | ||
Comment 46•9 years ago
|
||
Re-enabled on Beta40: https://hg.mozilla.org/releases/mozilla-beta/rev/dc2d67d504c7
status-firefox42:
--- → disabled
Reporter | ||
Comment 47•8 years ago
|
||
Markus, did you want to take a look at this at some point now that smichaud is gone?
Flags: needinfo?(mstange)
Reporter | ||
Comment 48•8 years ago
|
||
Actually, looks like this went away on its own at some point. I'll push a patch to re-enable it soon.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(mstange)
Resolution: --- → WORKSFORME
Whiteboard: [test disabled on OSX 10.10 debug][leave open]
Reporter | ||
Comment 49•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/596388fc0ff4 https://hg.mozilla.org/releases/mozilla-aurora/rev/7ede46ba6297
Comment 50•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/596388fc0ff4
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•