Closed
Bug 1200778
Opened 9 years ago
Closed 9 years ago
[APZ] Content response broken after dragging a tab out to create a new window
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mstange, Assigned: kats)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce (Firefox Desktop):
1. Increase the content response timeout to three seconds by setting apz.content_response_timeout to 3000.
2. Open a new tab and load http://www.apple.com/pr/library/2015/07/21Apple-Reports-Record-Third-Quarter-Results.html in it. (This page has a touchstart event handler.)
3. Observe that scrolling works correctly.
4. Drag the current tab downwards and drop it so that a new window is created from it.
5. Attempt to scroll in the new window.
Now scrolling doesn't work during the first three seconds of a scroll gesture.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bugmail.mozilla
Assignee | ||
Comment 1•9 years ago
|
||
The cached reference that the RenderFrameParent holds to the APZCTreeManager becomes stale. When a tab is reparented to a new window, the layers id associated with the RenderFrameParent becomes owned by a different CompositorParent but RenderFrameParent never realizes this. Therefore it keeps sending messages to the old APZCTreeManager which ignores them. In this case the main thread timeout has to fire before the page scrolls because the wrong APZCTreeManager gets the target-apzc notification from content.
The attached patch forces the RenderFrameParent to pick up a new APZCTreeManager when it gets moved to a new widget. I think technically this patch may introduce a race because the SendAdoptChild notification just above is async and it may be that GetApzcTreeManager() gets called before that notification is processed. However I think this is pretty unlikely (maybe impossible) in practice and in the future (bug 1020199) all of this will move out of RenderFrameParent anyway so I don't really want to add a new parent -> child API to notify the RenderFrameParent of when the switch is complete. Let me know if you would rather I did that anyway.
Attachment #8661186 -
Flags: review?(mstange)
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> The attached patch forces the RenderFrameParent to pick up a new
> APZCTreeManager when it gets moved to a new widget. I think technically this
> patch may introduce a race because the SendAdoptChild notification just
> above is async and it may be that GetApzcTreeManager() gets called before
> that notification is processed.
Hmm, how dangerous is this? If this happens, will GetApzcTreeManager() return null, or the old tree manager? If it's the former, I wouldn't be worried, but if it's the latter, doesn't that mean that we can get a dangling pointer if the old window is closed?
Assignee | ||
Comment 3•9 years ago
|
||
Hm, it would probably return the old tree manager. If the old window is subsequently closed, it would leak the APZCTreeManager instance since RenderFrameParent keeps a refptr to it. It wouldn't be a dangling pointer in the sense of use-after-free though.
Reporter | ||
Comment 4•9 years ago
|
||
Ok, that's not the end of the world then, but also not ideal. But we can punt on getting this right until it shows up as a real problem.
Reporter | ||
Updated•9 years ago
|
Attachment #8661186 -
Flags: review?(mstange) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•