1296229 - Crash in MozCrashErrorReporter

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This bug was filed from the Socorro interface and is 
report bp-b3bccf15-72d6-4cc3-aef1-c69ba2160817.
=============================================================

MOZ_CRASH Reason 	MOZ_CRASH(Why is someone touching JSAPI without an AutoJSAPI?)

Firefox 48.0 64 bit on Windows 8.1
Had a big browser session open and the browser idle when it crashed suddenly.

This message has been implemented as part of bug 1252565.

Comment 1

[Bobby Holley (:bholley)]

This looks like a regression from bug 1246153. Was there a reason we didn't use an AutoJSAPI there?

Saving NI until Boris is back on Monday.

Comment 2

[Boris Zbarsky [:bzbarsky]]

There were sort of reasons, to do with rooting.  Plus we were clearing exceptions... but I forgot that the JS engine would happily report exceptions, instead of setting them as pending, before it returned to us.

In any case, this code uses AutoJSAPI as of the fix for bug 1282150.  So in Firefox 50 this will become a nonissue... sort of.  Instead of a crash in the error reporter the OOM will just propagate as a failure back to DOMStorageManager::Observe which will then not act on the notification.  I'm not sure what the upshot of _that_ is.

In any case, we can backport the AutoJSAPI bit of bug 1282150 to 48/49 if we think it's worth it.  It should be reasonably safe, I think.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, I meant to ask Honza what the impact of DOMStorageManager::Observe not doing its work is.

Comment 4

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Need to decide whether to backport the fix.

Comment 5

[Honza Bambas (:mayhemer)]

The notification in the crash report is only a test notification (only consumer is a certain test, not any production code).  The aOriginAttributesPattern in that case is an empty string.  I think we can just bypass call to pattern.Init() for empty strings, according the OriginAttributesPatternDictionary ctor, it's already inited as empty.

However, when this is delete-by-pattern operation invocation, then we may have a problem.  The data sub-set defined by the pattern will not be deleted, which could be considered a privacy issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, can we come up with a safer callback for when we OOM on dealing with the dictionary?

Comment 7

[Honza Bambas (:mayhemer)]

Not sure what you mean with "safer callback".  The pattern, when constructed, is passed to OriginAttributes::Match that says if we have an origin attributes match and have to perform a jar deletion.  No pattern, no match.

I think this is not isolated to DOMStorage, we do this all over the code base when the "delete origin specific data" global notification is fired.  

The pattern is carried as a JSON string instead of a final object.  We must parse it in every implementation.  If we can change that - pass OriginAttributesPattern as an object - we can fail on a single place and notify user soon that "we can't delete you data, because we don't have memory for it".  And the observer callbacks won't have to deal with the parsing and handle its failure.

Honestly, the way OriginAttributes* are designed only brought problems hard to overcome in the past, and here is another one.  These classes may look easy to use in JS (somewhat) but are extremely complicated to use in C++ (thread safety caused by dependency on JS, no simple IDL support), despite they're actually concrete C++ classes...

Hence, passing the object directly may be hard.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, I meant "safer fallback".  As in, a safer fallback behavior when we fail to parse the JSON string due to oom.

Comment 9

[Gerry Chang [:gchang]]

Track 48+/49+ as this is a regression and crash.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Er, I meant "safer fallback".  As in, a safer fallback behavior when we fail
> to parse the JSON string due to oom.

I don't think there is a way.  We can leave it as is (leave the data on error) or delete everything (I think even more unfortunate).

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 49, it is not too high volume for now though not great either (36 crashes in 49 beta 6 in the last week.)

Comment 12

[Andrew Overholt [:overholt]]

Given we don't want to fix this this late in the 49 cycle and >= 50 isn't affected, should we just WONTFIX this?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well, it's fixed, by bug 1282150.  ;)