Going all to way to execution to find out that an underlying connection went away would be an strange API. It is better to use higher level logic (like one of the xpcom or profile change messages) and don't create the statement in the first place.

An initial try push is at https://tbpl.mozilla.org/?tree=Try&rev=d56a6d4b0d54

Shouldn't there be other changes in storage too? Since MOZ_ASSERT is a no-op in non-debug builds, it seems like your change either creates a possible null-pointer dereference in storage or the assert and the ensure-true are already superfluous...

(In reply to Andrew Sutherland (:asuth) from comment #2) > Shouldn't there be other changes in storage too? Since MOZ_ASSERT is a > no-op in non-debug builds, it seems like your change either creates a > possible null-pointer dereference in storage or the assert and the > ensure-true are already superfluous... They are unlikely to be hit right now. The reason is that we don't normally wait for tasks to run during shutdown. I found this failing while trying to fix that. Now that I think of it, this bug should probably depend on 721603, not the other way around.

I still think the code would be saner if we'd still "if (!target) {return NS_ERROR_NOT_AVAILABLE}" after moz_assert, we can always make mistakes that may unluckily not be noticed soon enough in the cycle. That said having a fatal assertion instead of a weak warning is a win. Could you add any meaningful comment to the assert to help figuring out what may have gone wrong in the code change?

I added comment and a reference to a bug that assert found.

though, this still doesn't reintroduce the null check. an ad-on may hit the same bug and crash us in opt builds, the assert wouldn't protect us.

My preference is for the previous version, as crashes are fairly well behaved bugs. If we must have keep the if, this patch does it.

Comment on attachment 599287 [details] [diff] [review] new patch Review of attachment 599287 [details] [diff] [review]: ----------------------------------------------------------------- if this is protecting a developer error the assert is fine, crashing users may help finding it earlier, but it's not usually nice vs the users, imo.

A new try is at https://tbpl.mozilla.org/?tree=Try&rev=291bc14461c2 Mak, given the resolution for 728653, is your r+ still standing?

sure, this doesn't change the behavior, just makes the error visible to us.

looks like i found a bug already. Debugging :-(

A new try push is at https://tbpl.mozilla.org/?tree=Try&rev=1eeb55426b70