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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

Attachments

(1 file)

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: nobody → bugmail.mozilla
Attached patch Patch (deleted) — Splinter Review
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)
(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?
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.
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.
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.

Attachment

General

Created:
Updated:
Size: