Closed
Bug 1328066
Opened 8 years ago
Closed 8 years ago
[APZ] Page doesn't redraw on scroll if I resized the window
Categories
(Core :: Panning and Zooming, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: arni2033, Assigned: kats)
References
Details
(Keywords: regression, Whiteboard: [gfx-noted])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
mstange
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160526082509 STR_1: 1. Open new window. Switch that window to normal (not maximized mode) 2. Open new window, paste "http://example.org/" in urlbar, press Enter 3. Resize the window by dragging its bottom border to the top, so that the page became scrollable 4. Hover mouse over the page and rotate mouse wheel down AR: White rectangle with text on the page is partially clipped ER: Page should redraw in a normal way This is regression from bug 1216924. Regression range: > https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a8ae52b3b96cb69f6253390aa61f08698c94fabd&tochange=fbe36b282024d49c78a5699d8e9e2824ef03c85a@ Kartikaya Gupta (email:kats@mozilla.com): It seems that this is a regresion caused by your change. Please have a look.
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Comment 1•8 years ago
|
||
The regression from bug 1216924
Flags: needinfo?(bugmail)
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee | ||
Comment 2•8 years ago
|
||
Dropping needinfo, I'll look at this again when I triage the rest of the bugs arni filed that ended up in the APZ component. (Welcome back arni!)
Flags: needinfo?(bugmail)
OS: Unspecified → Windows
Priority: P3 → --
Version: Trunk → 47 Branch
Assignee | ||
Comment 3•8 years ago
|
||
I can repro. This is effectively perma-checkerboarding, which is bad. Not sure offhand what's causing it.
Assignee: nobody → bugmail
status-firefox50:
--- → wontfix
status-firefox51:
--- → fix-optional
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Priority: -- → P1
Assignee | ||
Comment 4•8 years ago
|
||
The regression range is wrong. This is actually a regression from bug 1192910. The behaviour did change in bug 1216924 in that it became easier to see because the displayport stopped getting aligned to tiles during resize. Prior to that the displayport would still be aligned to "virtual" tiles so you would have to resize the window smaller and scroll farther to see the perma-checkerboarding.
Assignee | ||
Comment 5•8 years ago
|
||
So, working backwards: bug 1192910 introduced the bug because it prevented SchedulePaint() from getting called at [1] if the displayport didn't change. This resulted in the full displayport not getting repainted after displayport suppression was turned off (it is turned on during the window resize process, and turned off when the resize ends). This by itself should not have been a problem, because the code at [2] is supposed to trigger the repaint when the suppression ends. But, it didn't. Why not? Because it uses whatever presShell is passed at the time. If turns out that if you have two windows open, and resize one of the windows, the displayport seems to be suppressed on the active tab of both windows and the sActiveSuppressDisplayport counter in the content process climbs to 2. When you stop resizing, the displayport is unsuppressed in both windows, but whichever is the last one to get unsuppressed is the one that has the SchedulePaint() called on it. So either we should stop spuriously suppressing the displayport in the windows that are not being resized, or we need to call SchedulePaint() on all the windows' active tabs. I think the former is preferred. [1] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/layout/base/nsLayoutUtils.cpp#1310 [2] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/gfx/layers/apz/util/APZCCallbackHelper.cpp#876
Assignee | ||
Comment 6•8 years ago
|
||
The displayport suppression is triggered by the code at [1]. This code runs when any window is resized, and broadcasts a message which is handled by the code at [2], but there are going to be n instances of [2] where n is the number of windows. So we should filter that broadcast message to be window-specific somehow. The same applies to OS X. [1] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/widget/windows/nsWindow.cpp#5576 [2] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/browser/base/content/tabbrowser.xml#4892
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. https://reviewboard.mozilla.org/r/102714/#review103448 Hmm, I don't really like how the widget reaches into the window and the tab parent. I'm not really sure why, I think it's because the widget is at a lower layer where it shouldn't really know all that much about what happens in the upper layers. It would be nice if the TabParent could register itself as a listener for window live resizes on the widget that it lives in, and then respond to live resize "events" (not sure what shape those events could take) by suppressing its own display port if it's the primary tab parent.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #8) > Hmm, I don't really like how the widget reaches into the window and the tab > parent. I'm not really sure why, I think it's because the widget is at a > lower layer where it shouldn't really know all that much about what happens > in the upper layers. That's a good point. > It would be nice if the TabParent could register itself as a listener for > window live resizes on the widget that it lives in, and then respond to live > resize "events" (not sure what shape those events could take) by > suppressing its own display port if it's the primary tab parent. However doing this seems a lot more complicated and error-prone. Not only would the widget have to keep a list of potentially hundreds of "live resize listeners", it would waste time notifying all of them when only one of ever needs to know. Also when tabs get reparented into different windows we'll have to deal with unregistering and re-registering, and making sure notifications don't get inadvertently dropped. Would it satisfy your concern if I extracted a "live resize listener" interface from nsITabParent, and expose that from nsIXULWindow? So instead of xulWindow->GetPrimaryTabParent, I'd be calling xulWindow->GetLiveResizeListeners, and this would (for now) always just return a list of length 1, containing the active tab parent (but as a nsILiveResizeListener or whatever interface). That still achieves the goal of decoupling stuff somewhat but prevents us from doing a lot of unnecessary work.
![]() |
||
Comment 10•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > So either we should stop spuriously suppressing the displayport in the > windows that are not being resized, or we need to call SchedulePaint() on > all the windows' active tabs. I think the former is preferred. In the observer handler code, could you compare 'Services.focus.activeWindow' to the right top level 'window' to detect the foreground native window containing the tab, and only repaint that? There might be corner cases there but generally seems like it would avoid repainting windows that aren't foreground.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #10) > In the observer handler code, could you compare > 'Services.focus.activeWindow' to the right top level 'window' to detect the > foreground native window containing the tab, and only repaint that? There > might be corner cases there but generally seems like it would avoid > repainting windows that aren't foreground. I'm worried about the corner cases here. For example, what if the active window changes while you're doing the resize? The suppression would be invoked on one TabParent but the unsuppression would happen on a different TabParent, and so we'd end up repainting the wrong one. In a multi-e10s world the two TabParents might even correspond to different content processes, and so we'd suppress one (and leave it suppressed) and unsuppress the other.
Comment 12•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > Would it satisfy your concern if I extracted a "live resize listener" > interface from nsITabParent, and expose that from nsIXULWindow? So instead > of xulWindow->GetPrimaryTabParent, I'd be calling > xulWindow->GetLiveResizeListeners, and this would (for now) always just > return a list of length 1, containing the active tab parent (but as a > nsILiveResizeListener or whatever interface). That still achieves the goal > of decoupling stuff somewhat but prevents us from doing a lot of unnecessary > work. This sounds good to me.
![]() |
||
Comment 13•8 years ago
|
||
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. clearing this since it appears you and markus have an agreement on a way forward.
Attachment #8824185 -
Flags: review?(jmathies)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. Thanks, I'll update the patch. Dropping review flag for the moment.
Attachment #8824185 -
Flags: review?(mstange)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2a082c9639a6dc0bdbc3eafb3c12fd963c2ec09
Assignee | ||
Comment 16•8 years ago
|
||
Let's try that again without the overzealous deletion of code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a829cc4692bfa352516a847d21d26c4f88ac8c03
Assignee | ||
Comment 17•8 years ago
|
||
Try looks good, except for the windows build failure. I fixed that locally.
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. https://reviewboard.mozilla.org/r/102714/#review108254 Is there a reason you made nsILiveResizer an XPIDL-style interface, other than consistency with the rest of the surrounding code? I'd prefer a regular C++-style interface, i.e. namespace mozilla { class LiveResizeListener { virtual void LiveResizeStarted()=0; virtual void LiveResizeStopped()=0; }; } And you could make getLiveResizeListeners a [noscript,notxpcom,...] method that retuns the array instead of using an outparam. You're not checking the return value for errors anyway. I'd like to take another look at the next revision of this patch, if you agree that this is worth doing.
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #19) > Is there a reason you made nsILiveResizer an XPIDL-style interface, other > than consistency with the rest of the surrounding code? I'd prefer a regular > C++-style interface, i.e. namespace mozilla { class LiveResizeListener { > virtual void LiveResizeStarted()=0; virtual void LiveResizeStopped()=0; }; } Mostly because I wasn't sure about mixing XPIDL interfaces with non-XPIDL interfaces. I did consider making it a C++ interface but figured that in order to get it refcounted and such properly I'd end up writing by hand a lot of the boilerplate that the generator would do for me anyway, so that seemed simpler. I can change it though. > And you could make getLiveResizeListeners a [noscript,notxpcom,...] method > that retuns the array instead of using an outparam. You're not checking the > return value for errors anyway. That's true. That approach does have a bit more overhead with constructing arrays and passing them around which is avoided in the current approach, where the array is just created once in nsBaseWidget and then populated/cleared repeatedly.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
^ This update makes LiveResizeListener a C++ class in the mozilla namespace. I changed the nsCOMPtr<>s to RefPtr. Landing bug 1312319 will make it a bit cleaner. I can change the function to create and return a nsTArray instead if you want that change. It'll then require copying the items out of the returned array and into mLiveResizeListeners in nsBaseWidget.
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. https://reviewboard.mozilla.org/r/102714/#review109386 Thanks!
Attachment #8824185 -
Flags: review?(mstange) → review+
Comment 24•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22) > I can change the function to create and return a nsTArray instead if you > want that change. It'll then require copying the items out of the returned > array and into mLiveResizeListeners in nsBaseWidget. Why copying and not moving?
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #24) > Why copying and not moving? Sure, we can do it as a move. Really I meant "some code will need to be written there" if we want to go with that approach.
Comment 26•8 years ago
|
||
Would this not work? nsTArray<RefPtr<mozilla::LiveResizeListener>> nsXULWindow::GetLiveResizeListeners() { nsTArray<RefPtr<mozilla::LiveResizeListener>> listeners; if (mPrimaryTabParent) { TabParent* parent = static_cast<TabParent*>(mPrimaryTabParent.get()); listeners.AppendElement(parent); } return listeners; } [...] mLiveResizeListeners = xulWindow->GetLiveResizeListeners();
Assignee | ||
Comment 27•8 years ago
|
||
Doh, that's what you meant. Sure, we can do that too. I'll update the patch.
Comment hidden (mozreview-request) |
Comment 30•8 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ff5cc310486 Don't broadcast the live-resize events to all browser windows unnecessarily. r=mstange
![]() |
||
Comment 31•8 years ago
|
||
Backed out for asserting in browser/components/sessionstore/test/browser_477657.js at widget/cocoa/nsChildView.mm:3668: https://hg.mozilla.org/integration/autoland/rev/4a43a7579d1cedfb72acbf55a06fa8101616178c Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6ff5cc31048644c02410da222b6626d771e93d1b Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73137948&repo=autoland 15:21:49 INFO - TEST-START | browser/components/sessionstore/test/browser_477657.js 15:21:49 INFO - ++DOCSHELL 0x12f1e6000 == 24 [pid = 1779] [id = {fff3b735-9dcb-eb4a-b0f9-bbf49860fe62}] 15:21:49 INFO - ++DOMWINDOW == 116 (0x12f538000) [pid = 1779] [serial = 234] [outer = 0x0] 15:21:49 INFO - ++DOMWINDOW == 117 (0x12f53b800) [pid = 1779] [serial = 235] [outer = 0x12f538000] 15:21:49 INFO - ++DOCSHELL 0x1300c1800 == 25 [pid = 1779] [id = {979e1d7e-c1ff-f749-bbd9-4411cd4909b7}] 15:21:49 INFO - ++DOMWINDOW == 118 (0x1300c2000) [pid = 1779] [serial = 236] [outer = 0x0] 15:21:49 INFO - ++DOCSHELL 0x1300ce000 == 26 [pid = 1779] [id = {cc6cea40-2a45-d54d-a192-58d52f32e091}] 15:21:49 INFO - ++DOMWINDOW == 119 (0x1300ce800) [pid = 1779] [serial = 237] [outer = 0x0] 15:21:49 INFO - ++DOCSHELL 0x1303b3000 == 27 [pid = 1779] [id = {7611d1b2-126b-8e42-9735-579d56cf8e25}] 15:21:49 INFO - ++DOMWINDOW == 120 (0x130474800) [pid = 1779] [serial = 238] [outer = 0x0] 15:21:49 INFO - ++DOMWINDOW == 121 (0x130a3b800) [pid = 1779] [serial = 239] [outer = 0x130474800] 15:21:49 INFO - ++DOCSHELL 0x1103a5800 == 5 [pid = 1780] [id = {8da2cb0e-caa2-7148-af99-404b0cab8612}] 15:21:49 INFO - ++DOMWINDOW == 34 (0x1191b1800) [pid = 1780] [serial = 111] [outer = 0x0] 15:21:49 INFO - ++DOMWINDOW == 35 (0x119c04800) [pid = 1780] [serial = 112] [outer = 0x1191b1800] 15:21:49 INFO - ++DOMWINDOW == 122 (0x131f18000) [pid = 1779] [serial = 240] [outer = 0x1300c2000] 15:21:49 INFO - ++DOMWINDOW == 123 (0x131f22800) [pid = 1779] [serial = 241] [outer = 0x1300ce800] 15:21:49 INFO - ++DOMWINDOW == 36 (0x119e8a800) [pid = 1780] [serial = 113] [outer = 0x1191b1800] 15:21:50 INFO - --DOCSHELL 0x12cd82000 == 5 [pid = 1781] [id = {a25cd313-6a96-694d-b74d-78e4d17c0b2b}] 15:21:50 INFO - --DOCSHELL 0x1211a0000 == 4 [pid = 1781] [id = {d728f4d4-9c3d-5949-8cba-a271a208a33d}] 15:21:50 INFO - --DOCSHELL 0x125c22000 == 3 [pid = 1781] [id = {347bf538-6a08-cd43-aab5-5ba7679cdff1}] 15:21:50 INFO - --DOCSHELL 0x114a28000 == 2 [pid = 1781] [id = {66ddbbe1-06c6-b54d-a918-89dbd9c452d5}] 15:21:50 INFO - --DOCSHELL 0x11c7f0000 == 1 [pid = 1781] [id = {e2a4904b-f1c7-e542-883f-c3abf8852c94}] 15:21:50 INFO - --DOCSHELL 0x126342000 == 0 [pid = 1781] [id = {84cef585-8a68-8146-afca-fe978e090ca8}] 15:21:50 INFO - --DOMWINDOW == 42 (0x11c7e3800) [pid = 1781] [serial = 39] [outer = 0x0] [url = about:blank] 15:21:50 INFO - --DOMWINDOW == 41 (0x11c7f2000) [pid = 1781] [serial = 40] [outer = 0x0] [url = about:blank] 15:21:50 INFO - --DOMWINDOW == 40 (0x1257d0000) [pid = 1781] [serial = 36] [outer = 0x0] [url = about:blank] 15:21:50 INFO - --DOMWINDOW == 39 (0x125a66000) [pid = 1781] [serial = 37] [outer = 0x0] [url = http://mochi.test:8888/browser/browser/components/sessionstore/test/browser_423132_sample.html] 15:21:50 INFO - Assertion failure: windowWidget, at /builds/slave/autoland-m64-d-000000000000000/build/src/widget/cocoa/nsChildView.mm:3668 15:21:50 INFO - #01: AppKit + 0x2888c9 15:21:50 INFO - 15:21:50 INFO - #02: AppKit + 0x288829 15:21:50 INFO - 15:21:50 INFO - #03: AppKit + 0x288829 15:21:50 INFO - 15:21:50 INFO - #04: AppKit + 0x2886a6 15:21:50 INFO - 15:21:50 INFO - #05: AppKit + 0x288635 15:21:50 INFO - 15:21:50 INFO - #06: AppKit + 0x28845d 15:21:50 INFO - 15:21:50 INFO - #07: AppKit + 0x265cf5 15:21:50 INFO - 15:21:50 INFO - #08: AppKit + 0x2876d6 15:21:50 INFO - 15:21:50 INFO - #09: AppKit + 0x18fdd9 15:21:50 INFO - 15:21:50 INFO - #10: AppKit + 0x2866e3 15:21:50 INFO - 15:22:21 INFO - #11: nsCocoaWindow::SetSizeMode(nsSizeMode) [widget/cocoa/nsCocoaWindow.mm:1277] 15:22:21 INFO - 15:22:21 INFO - #12: nsGlobalWindow::Maximize() [xpcom/base/nsCOMPtr.h:402] 15:22:21 INFO - 15:22:21 INFO - #13: mozilla::dom::WindowBinding::maximize [obj-firefox/dom/bindings/WindowBinding.cpp:6943] 15:22:21 INFO - 15:22:21 INFO - #14: mozilla::dom::WindowBinding::genericMethod [obj-firefox/dom/bindings/WindowBinding.cpp:15425] 15:22:21 INFO - 15:22:21 INFO - #15: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:240] 15:22:21 INFO - 15:22:21 INFO - #16: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:460] 15:22:21 INFO - 15:22:21 INFO - #17: <name omitted> [js/src/vm/Interpreter.cpp:524] 15:22:21 INFO - 15:22:21 INFO - #18: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/Wrapper.cpp:165] 15:22:21 INFO - 15:22:21 INFO - #19: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/CrossCompartmentWrapper.cpp:335] 15:22:21 INFO - 15:22:21 INFO - #20: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) [js/src/proxy/Proxy.cpp:421] 15:22:21 INFO - 15:22:21 INFO - #21: js::proxy_Call(JSContext*, unsigned int, JS::Value*) [js/public/RootingAPI.h:795] 15:22:21 INFO - 15:22:21 INFO - #22: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:240] 15:22:21 INFO - 15:22:21 INFO - #23: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:448] 15:22:21 INFO - 15:22:21 INFO - #24: Interpret [js/src/vm/Interpreter.cpp:2957] 15:22:21 INFO - 15:22:21 INFO - #25: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:406] 15:22:21 INFO - 15:22:21 INFO - #26: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:478] 15:22:21 INFO - 15:22:21 INFO - #27: <name omitted> [js/src/vm/Interpreter.cpp:524] 15:22:21 INFO - 15:22:21 INFO - #28: js::fun_apply(JSContext*, unsigned int, JS::Value*) [js/src/jsfun.cpp:1240] 15:22:21 INFO - 15:22:21 INFO - #29: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:240] 15:22:21 INFO - 15:22:21 INFO - #30: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:460] 15:22:21 INFO - 15:22:21 INFO - #31: Interpret [js/src/vm/Interpreter.cpp:2957] 15:22:21 INFO - 15:22:21 INFO - #32: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:406] 15:22:21 INFO - 15:22:21 INFO - #33: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:478] 15:22:21 INFO - 15:22:21 INFO - #34: <name omitted> [js/src/vm/Interpreter.cpp:524] 15:22:21 INFO - 15:22:21 INFO - #35: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/Wrapper.cpp:165] 15:22:21 INFO - 15:22:21 INFO - #36: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/CrossCompartmentWrapper.cpp:335] 15:22:21 INFO - 15:22:21 INFO - #37: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) [js/src/proxy/Proxy.cpp:421] 15:22:21 INFO - 15:22:21 INFO - #38: js::proxy_Call(JSContext*, unsigned int, JS::Value*) [js/public/RootingAPI.h:795] 15:22:21 INFO - 15:22:21 INFO - #39: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:240] 15:22:21 INFO - 15:22:21 INFO - #40: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:448] 15:22:21 INFO - 15:22:21 INFO - #41: <name omitted> [js/src/vm/Interpreter.cpp:524] 15:22:21 INFO - 15:22:21 INFO - #42: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2847] 15:22:21 INFO - 15:22:21 INFO - #43: mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) [obj-firefox/dom/bindings/FunctionBinding.cpp:36] 15:22:21 INFO - 15:22:21 INFO - #44: void mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) [obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:71] 15:22:21 INFO - 15:22:21 INFO - #45: nsGlobalWindow::RunTimeoutHandler(mozilla::dom::Timeout*, nsIScriptContext*) [dom/bindings/ErrorResult.h:295] 15:22:21 INFO - 15:22:21 INFO - #46: mozilla::dom::TimeoutManager::RunTimeout(mozilla::dom::Timeout*) [dom/base/TimeoutManager.cpp:552] 15:22:21 INFO - 15:22:21 INFO - #47: mozilla::dom::(anonymous namespace)::TimerCallback(nsITimer*, void*) [mfbt/RefPtr.h:40] 15:22:21 INFO - 15:22:21 INFO - #48: nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:475] 15:22:21 INFO - 15:22:21 INFO - #49: nsTimerEvent::Run() [mfbt/RefPtr.h:62] 15:22:21 INFO - 15:22:21 INFO - #50: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1261] 15:22:21 INFO - 15:22:21 INFO - #51: NS_ProcessPendingEvents(nsIThread*, unsigned int) [xpcom/threads/nsThreadUtils.cpp:331] 15:22:21 INFO - 15:22:21 INFO - #52: nsBaseAppShell::NativeEventCallback() [widget/nsBaseAppShell.cpp:98] 15:22:21 INFO - 15:22:21 INFO - #53: nsAppShell::ProcessGeckoEvents(void*) [widget/cocoa/nsAppShell.mm:393] 15:22:21 INFO - 15:22:21 INFO - #54: CoreFoundation + 0x80a01 15:22:21 INFO - 15:22:21 INFO - #55: CoreFoundation + 0x72c5c 15:22:21 INFO - 15:22:21 INFO - #56: CoreFoundation + 0x721bf 15:22:21 INFO - 15:22:21 INFO - #57: CoreFoundation + 0x71bd8 15:22:21 INFO - 15:22:21 INFO - #58: HIToolbox + 0x3256f 15:22:21 INFO - 15:22:21 INFO - #59: HIToolbox + 0x322ea 15:22:21 INFO - 15:22:21 INFO - #60: HIToolbox + 0x3212b 15:22:21 INFO - 15:22:21 INFO - #61: AppKit + 0x918ab 15:22:21 INFO - 15:22:21 INFO - #62: AppKit + 0x90e58 15:22:21 INFO - 15:22:21 INFO - #63: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [widget/cocoa/nsAppShell.mm:128] 15:22:21 INFO - 15:22:21 INFO - #64: AppKit + 0x86af3 15:22:21 INFO - 15:22:21 INFO - #65: nsAppShell::Run() [xpcom/base/nsCOMPtr.h:551] 15:22:21 INFO - 15:22:21 INFO - #66: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:283] 15:22:21 INFO - 15:22:21 INFO - #67: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4461] 15:22:21 INFO - 15:22:21 INFO - #68: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4638] 15:22:21 INFO - 15:22:21 INFO - #69: XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4729] 15:22:21 INFO - 15:22:21 INFO - #70: main [browser/app/nsBrowserApp.cpp:234] 15:22:21 INFO - 15:22:21 INFO - Hit MOZ_CRASH(Aborting on channel error.) at /builds/slave/autoland-m64-d-000000000000000/build/src/ipc/glue/MessageChannel.cpp:2186
Flags: needinfo?(bugmail)
Comment 32•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22) > Landing bug 1312319 will make it a bit cleaner. Bug 1312319 has landed, so you could take the opportunity and make this change before relanding.
Assignee | ||
Comment 33•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #32) > Bug 1312319 has landed, so you could take the opportunity and make this > change before relanding. Good point. Ironic that I forgot to do this before :/
Flags: needinfo?(bugmail)
Assignee | ||
Comment 34•8 years ago
|
||
The assertion I added was too restrictive. It's totally possible for viewWillStartLiveResize and viewDidEndLiveResize to be called when mGeckoChild is null. I had added a null guard in viewWillStartLiveResize but an assertion in viewDidEndLiveResize, hoping to catch cases where the mGeckoChild goes away between the two calls, but it also tripped on cases where mGeckoChild was null throughout. Thinking about this more I'm not sure it's worth adding an assertion here because to do it properly I would have to set a flag in viewWillStartLiveResize as to whether or not the window was found, then then assert that the flag is not set in viewDidEndLiveResize if no window is found. Seems unnecessarily complex, given that mGeckoChild is only nulled out in |widgetDestroyed| at which point we don't really care about balancing the calls because the nsBaseWidget::Shutdown code will do the cleanup anyway. I kicked off a fresh try push without the assertion and with the patch rebased to master with the NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=470dd3d4ec574d1a0ace744c0720e9af5fe43c7b
Comment 35•8 years ago
|
||
Removing the assertion and handling non-balanced calls somewhat gracefully sounds good to me.
Comment 36•8 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/37a01852d420 Don't broadcast the live-resize events to all browser windows unnecessarily. r=mstange
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/37a01852d420
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 38•8 years ago
|
||
This looks kinda big and scary. Worth trying to uplift to Aurora and/or Beta?
Flags: needinfo?(bugmail)
Assignee | ||
Comment 39•8 years ago
|
||
It's not *too* scary. I think we can skip beta but it might be worth uplifting to aurora. I'll give it a few days to see if anybody files any regressions.
Flags: needinfo?(bugmail)
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. Approval Request Comment [Feature/Bug causing the regression]: bug 1192910 [User impact if declined]: in some cases after resizing a window and scrolling the user sees checkerboarding that doesn't go away on its own [Is this code covered by automated tests?]: not really [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: yes please. STR in comment 0 [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low-medium risk [Why is the change risky/not risky?]: it's a large patch but conceptually not too complicated. basically replacing with data-propagation mechanism with another that's more tightly scoped. no regressions reported so far on Nightly, so I'm not too worried. However I'm not recommending uplift to beta just because it's a hefty patch. [String changes made/needed]: none
Attachment #8824185 -
Flags: approval-mozilla-aurora?
Comment 41•8 years ago
|
||
Hi Florin, could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
Comment 42•8 years ago
|
||
Moving to Petruta who's been working on APZ testing for a while.
Flags: needinfo?(florin.mezei) → needinfo?(petruta.rasa)
I've reproduced this issue on latest Aurora 53.0a2 under Win 10 64-bit and Mac OS X 10.11.6, but not under Ubuntu 14.04 32-bit. On latest Nightly 54.0a1 the issue is fixed.
Comment 44•8 years ago
|
||
Comment on attachment 8824185 [details] Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily. Fix an APZ regression and was verified. Aurora53+.
Attachment #8824185 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 45•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/84303d94533c
Comment 46•8 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=76844190&repo=mozilla-aurora
Flags: needinfo?(bugmail)
Assignee | ||
Comment 47•8 years ago
|
||
Whoops. That macro is only on 54, but I inlined it for the uplift. Relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/cab6dae5be9dfc0304c2a9325af12cdab373cc31
Flags: needinfo?(bugmail)
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify+
Verified as fixed with Firefox 53 beta 1 under Win 10 64-bit.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•