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

RESOLVED FIXED in Firefox 54

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: marcia, Assigned: asuth)

Tracking

({crash, regression})

Trunk
mozilla54
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

(crash signature)

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
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.)
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.
Flags: needinfo?(afarre)
See Also: → 1336229
(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"))
(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.
Blocks: 1285898
Flags: needinfo?(afarre) → needinfo?(asuth)
Keywords: regression
See Also: → 1336323
Component: XPCOM → DOM
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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Summary: Crash in NS_strcmp → LocalStorage bfcache FireDelayedDOMEvents causes null-pointer reads [Crash in NS_strcmp]
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(asuth)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.