1336242 - LocalStorage bfcache FireDelayedDOMEvents causes null-pointer reads [Crash in NS_strcmp]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-f75e0395-b72e-4d29-9d08-ce5852170202.
=============================================================

Seen while looking at trunk crash stats - crashes started using 20170202030211 build: http://bit.ly/2jCBO9J

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d025ac534a6333a8170a59a95a8a3673d4028ee&tochange=f985243bb630b2c78cd57731c8d8ab191aa09527

Comment 1

[Andrew McCreight [:mccr8]]

These stacks look pretty bogus. On 54, I see 152 crashes that supposedly are coming from nsImageRenderer::ComputeObjectAnchorPoint, though I don't see how that could happen. 80 via  MD_CURRENT_THREAD, which also doesn't make sense. Then another 60 or so via nsGlobalWindow::FireDelayedDOMEvents, which maybe is possible. (I actually hit this variant myself. I had just closed a tab or switched tabs, then hit back a bunch of times.)

Comment 2

[Andrew McCreight [:mccr8]]

Here's an example of a crash inside FireDelayeDOMEvents:
  bp-489caca2-e9b2-4a56-887f-485d92170202

Andreas, could this be related to bug 1313864? It is in the regression range and at least sounds similar.

Comment 3

[Toshihiro Yamada]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Here's an example of a crash inside FireDelayeDOMEvents:
>   bp-489caca2-e9b2-4a56-887f-485d92170202
> 
> Andreas, could this be related to bug 1313864? It is in the regression range
> and at least sounds similar.

I'm not sure, but I think this crash is derived from Bug 1285898.
In nsGlobalWindow::FireDelayedDOMEvents(), nsGlobalWindow::Observe is called with *aData = nullptr and hit crash in Observe (line 11823) if (!NS_strcmp(aData, u"sessionStorage"))

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Toshihiro Yamada from comment #3)
> I'm not sure, but I think this crash is derived from Bug 1285898.
> In nsGlobalWindow::FireDelayedDOMEvents(), nsGlobalWindow::Observe is called
> with *aData = nullptr and hit crash in Observe (line 11823) if
> (!NS_strcmp(aData, u"sessionStorage"))

Ah, good catch! Yes, that makes sense.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Indeed, excellent detective work!

The ironic thing is that those pending storage events don't even work right.  Never have[1].  And I was just about to file the spin-off bug for that since I noticed them in bug 1285898.  Also, they apparently leak.  I don't see where we clear mPendingStorageEvents.

I'm half tempted to just nuke the broken feature...

1: At least, not for content that wants the standard "storage" events.  I think the crash makes it clear we have no automated tests covering the bfcache case, but there could be some since the localStorage tests I found seem to use a hacky simplification by depending on the "MozLocalStorageChanged" event sent for events sourced from the self-same document.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I had philor backout bug 1285898 and I'll fix-up the bfcache case and add a test before we re-land.  Backout noted at https://bugzilla.mozilla.org/show_bug.cgi?id=1285898#c36 and is commit https://hg.mozilla.org/mozilla-central/rev/2aede0a97bc685e163196cc451b947a04ae6a598.