We should probably use a strong assert for target in AsyncExecuteStatements::execute

RESOLVED FIXED in mozilla13

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
No longer blocks: 721603
Created attachment 597972 [details] [diff] [review]
use MOZ_ASSERT

https://tbpl.mozilla.org/?tree=Try&rev=547bdbe7082f
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #597972 - Flags: review?(mak77)
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?
Created attachment 599187 [details] [diff] [review]
With added comment

I added comment and a reference to a bug that assert found.
Attachment #597972 - Attachment is obsolete: true
Attachment #597972 - Flags: review?(mak77)
Attachment #599187 - Flags: review?(mak77)
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.
Created attachment 599287 [details] [diff] [review]
new patch

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.
Attachment #599187 - Attachment is obsolete: true
Attachment #599187 - Flags: review?(mak77)
Attachment #599287 - Flags: review?(mak77)
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.
Attachment #599287 - Flags: review?(mak77) → review+
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 :-(
No longer depends on: 733358
https://hg.mozilla.org/mozilla-central/rev/5317a517118f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.