637679 - Don't notify directly in AsyncGetBookmarksForURI::HandleResult

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This has potential to cause bad happening if the observer does something funny like spinning the events loop or similar.
Rather than than, collect an array of ids and pass it to the notifying method, then simply for loop it.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

This adds some indirection that should solve the issues caused by observers spinning the event loop.
I pass results by value only once in HandleCompletion, so that the notifying function only relies on what is in its stack rather than on pointers to stuff that could disappear.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=91a962414422

builds to test this fix will appear in:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-91a962414422

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 515951 [details] [diff] [review]
patch

So far this has been bulletproof on my local testing, but since the crashes are happening at "random" times, I won't call this victory till I can't crash after some hundred tries.

Nonetheless, since this block a hardblocker, I prefer starting asking for review earlier so that we can move on quickier on alternative ways if needed.

Comment 4

[Shawn Wilsher :sdwilsh]

I think it would be safer and simpler to just put an nsCOMPtr (strong reference) to the callback in CallbackResultNotifier (in storage).  That would ensure that the callback exists for the lifetime of the callback.  It'd also be testable.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

still, this also ensures correct order of the events, and avoids to spin the loop while we are in the middle of a CallbackResultNotifier::Run...

Could we do both? so fix this in Places and add the strong ownership in a Storage bug?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the comment at http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageAsyncStatementExecution.cpp#461 seems strange, completion notifier does not take ownership afaict.

Adding ownership to the notifiers also means they could be the last thing owning the callback, this means they should proxyRelease it to their respective thread.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ah I see, mCallback is AddRefed when AsyncExecuteStatements is created, and then Released by CompletionNotifier

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I filed Storage bug 637957.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 515951 [details] [diff] [review]
patch

Based on latest analysis, I think I can simplify this patch to a point where the only cost will be a single nsTArray<PRInt64> addition to the object.

I'd still collect ids in an array in HandleResult, in HandleCompletion I can directly loop the array and notify without changing anything other in the code, at this point even if something does bad jokes with events the object cannot be released since it's owned by CompletionNotifier.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.0

I think this is much simpler and acceptable.
Unless somebody looks at it before me, tomorrow I could look to the Storage bug, but I globally would be less worried with this Places patch.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 516126 [details] [diff] [review]
patch v2.0

r=sdwilsh, but I suspect we'll just want to wait to take this for Firefox 5 to not introduce any risk before we hit RC.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ok, we can wait if crash stats doesn't show the need of further fixes.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thinking and rethinking, I'd say this can be wontfixed.
The crash fix in Storage works pretty fine and it 100% analyzed, fixing this brings to use a bit more memory for the temp array.
For now let's just forget it.