Bug 8360 – Repro crash when onscroll handler deletes the scrolled object

• Component: Layout and Rendering
• Status: RESOLVED FIXED

Follow-up to bug 7048, to track the remaining cases where an object is accessed after calling its onscroll handler, which may have deleted it.

Created attachment 7673 [details]
Test case (crashes)

The second one doesn't crash, but I think it's only pure chance that it doesn't. There is one more crashing opportunity, in Marquee::start, which this test case doesn't cover.

[Darin Adler]

This is the kind of thing that's quite difficult to fix without reference counting, and easy to fix with reference counting. We should consider adding reference counts to RenderLayer objects to fix this.

[Alice Liu]

<rdar://problem/4556764>

[Darin Adler]

I think the way to fix this is to go from the RenderLayer to the RenderObject and then from the RenderObject to that RenderObject's Element from the DOM. We can hold a referenced pointer to that element. Then we can get back to the layer from the element after the fact. If we can't get back, then we know the layer was destroyed.

The only problem with that would be if there can be a layer for an object which does not correspond to an element (generated content). If there can, then this approach won't work. Otherwise, it's very easy to code. I'm tempted to code it up and then ask Hyatt if it's OK or not.

(In reply to comment #4)
> The only problem with that would be if there can be a layer for an object which
> does not correspond to an element (generated content).

Something like this?

<style>
	div:before { content:'bar'; position: absolute; }
</style>
<div>foo</div>

Created attachment 12569 [details]
Delay dispatching scroll events until after layout

Comes with a layout test and a change log

[Darin Adler]

Comment on attachment 12569 [details]
Delay dispatching scroll events until after layout

The dispatchScrollEvent function should be private, not public.

Can we test this?

r=me

(In reply to comment #7)
> The dispatchScrollEvent function should be private, not public.

It is:
@@ -377,6 +377,8 @@ private:

> Can we test this?

The patch includes a test.

[Sam Weinig]

Landed in r19005.

I think the patch had a negative effect on the Safari RSS sidebar and probably many sites that use JavaScript to keep elements in a fixed position.

[Darin Adler]

(In reply to comment #10)
> I think the patch had a negative effect on the Safari RSS sidebar and probably
> many sites that use JavaScript to keep elements in a fixed position.

Lets roll it out and reopen the bug. Sam, would you do that?

(In reply to comment #11)
> Lets roll it out and reopen the bug. Sam, would you do that?
> 

Filed bug 12354

[Sam Weinig]

Patch rolled out in r19016.

[Sam Weinig]

Reopening bug!

[Sam Weinig]

Comment on attachment 12569 [details]
Delay dispatching scroll events until after layout

Marking r- as it caused regressions.

Created attachment 12704 [details]
Delay dispatching scroll events until after layout

Turns out FrameView already has a mechanism for delaying events.

[David Kilzer (:ddkilzer)]

Created attachment 12708 [details]
Delay dispatching scroll events until after layout (r3)

I'm pretty sure Mitz wanted this test to dump as text, so I changed the layout test from Attachment 12704 [details] to do just that.

[Darin Adler]

Comment on attachment 12708 [details]
Delay dispatching scroll events until after layout (r3)

Looks fine, r=me.

[David Kilzer (:ddkilzer)]

Committed revision 19204.

[David Kilzer (:ddkilzer)]

Committed revision 19205.  (Fixed ChangeLog entry to remove files that weren't committed.)