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)

Product:
Core
Component:
Panning and Zooming
Version:
47 Branch
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- verified
firefox54 --- verified

People

(Reporter: arni2033, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

Bug 1328066 - Don't broadcast the live-resize events to all browser windows unnecessarily.
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.
No longer blocks: 1277113
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
The regression from bug 1216924
Flags: needinfo?(bugmail)
Priority: -- → P3
Whiteboard: [gfx-noted]
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
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
See Also: → 1327095
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.
Blocks: 1192910
No longer blocks: 1216924
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
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 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.
(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.
(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.
(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.
(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 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)
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)
Let's try that again without the overzealous deletion of code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a829cc4692bfa352516a847d21d26c4f88ac8c03
Try looks good, except for the windows build failure. I fixed that locally.
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.
(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.
^ 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 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+
(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?
(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.
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();
Doh, that's what you meant. Sure, we can do that too. I'll update the patch.
^ patch updated, carrying r+
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
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)
(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.
(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)
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
Removing the assertion and handling non-balanced calls somewhat gracefully sounds good to me.
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
https://hg.mozilla.org/mozilla-central/rev/37a01852d420
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
This looks kinda big and scary. Worth trying to uplift to Aurora and/or Beta?
status-firefox51: fix-optional → wontfix
Flags: needinfo?(bugmail)
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.
status-firefox52: affected → wontfix
Flags: needinfo?(bugmail)
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?
Hi Florin,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
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.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(petruta.rasa)
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/84303d94533c
status-firefox53: affected → fixed
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=76844190&repo=mozilla-aurora
status-firefox53: fixed → affected
Flags: needinfo?(bugmail)
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)
Flags: qe-verify+
Verified as fixed with Firefox 53 beta 1 under Win 10 64-bit.
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: