Closed Bug 637679 Opened 10 years ago Closed 10 years ago
Don't notify directly in Async
Get Bookmarks For URI::Handle Result
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.
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.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=91a962414422 builds to test this fix will appear in: http://email@example.com
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.
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.
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?
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.
ah I see, mCallback is AddRefed when AsyncExecuteStatements is created, and then Released by CompletionNotifier
I filed Storage bug 637957.
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.
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.
Attachment #516126 - Flags: review?(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.
Attachment #516126 - Flags: review?(sdwilsh) → review+
ok, we can wait if crash stats doesn't show the need of further fixes.
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.