629950 - [@ nsIPresShell::RemoveRefreshObserverInternal(nsARefreshObserver*, mozFlushType)]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=nsIPresShell%3A%3ARemoveRefreshObserverInternal%28nsARefreshObserver*%2C%20mozFlushType%29&version=Firefox%3A4.0b11pre

There are some crashes on trunk. #18 in the topcrash list atm.

Comment 1

[alexander :surkov (:asurkov)]

Boris, is it unsafe to remove refresh observer when pagehide event is handled?

Comment 2

[Boris Zbarsky [:bzbarsky]]

All those crashes are at offset 0x20 == 32 and in 32-bit builds.  And the code it crashes on is:

  GetPresContext()->RefreshDriver()->
    RemoveRefreshObserver(aObserver, aFlushType); 

32 is the offset of mRefreshDriver inside nsPresContext on 32-bit systems.  So GetPresContext() is returning null.  And yes, that could happen during pagehide...

nsIPresShell::RemoveRefreshObserverInternal should  null-check the prescontext.  Probably the add method should too.

Alexander, want to write the the patch?

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

sure.

asking approval until bug is marked as blocking

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 508677 [details] [diff] [review]
patch

I'd prefer a local for the prescontext.  With that change, r+a=me.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch3 [for landing]

with bz's comment addressed

Comment 6

[alexander :surkov (:asurkov)]

landed on 2.0 beta 11 - http://hg.mozilla.org/mozilla-central/rev/8b5cb26bbb10