Closed Bug 721582 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Storage, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch use MOZ_ASSERT (obsolete) — Splinter Review
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?
Attached patch With added comment (obsolete) — Splinter Review
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.
Attached patch new patchSplinter Review
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.