Closed Bug 637679 Opened 10 years ago Closed 10 years ago

Don't notify directly in AsyncGetBookmarksForURI::HandleResult

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
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 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.
Attachment #515951 - Flags: review?(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.
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.
Attachment #515951 - Attachment is obsolete: true
Attachment #515951 - Flags: review?(sdwilsh)
Attached patch patch v2.0Splinter Review
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.