moving this patch out of bug 511860, it was r=sdwilsh, so i'm forwarding the review here. see https://bugzilla.mozilla.org/show_bug.cgi?id=511860#c3 and https://bugzilla.mozilla.org/show_bug.cgi?id=511860#c4 for reasoning.

backed out, looks like this is causing all of our random crashes to become permanent on tinderboxes. And those crashes are not locally reproduceable, nor we have a stack trace for them. http://hg.mozilla.org/mozilla-central/rev/dbabc8dd62b6

pushed to Places branch for testing: http://hg.mozilla.org/projects/places/rev/45c3d6bf12b3

Ok, the patch is forcing the crashes on Places Branch, would it be possible to coordinate with IT to get a stack trace on a Places Unit test box?

If you temporarily land this in Places you should get stacks in the full Unittest builds. I suspect this isn't the right way to fix it permanently, and it probably doesn't work for the packaged test case. When I was testing I hit crashes in test_browserGlue_shutdown.js rather than test_384370.js, test_421483.js and test_bookmarks_html.js. That's probably because it took a bit of experimentation to get to this patch. Did have to run all of xpcshell-tests to get the crash though, not just test_browser_places/, so there's some sort of inter-test interaction going on.

So, i've landed temporarly bug 506042 (that should be the same), and waiting to see if the stack crash we get is the same as in bug 507199, sounds like it could be.

so, yes, the stack trace is the same as bug 507199: Thread 8 (crashed) 0 xul.dll!mozilla::storage::Statement::Finalize() [mozStorageStatement.cpp:443f7fdb70f7 : 400 + 0x3] eip = 0x0099d430 esp = 0x0302fe7c ebp = 0x0302fe94 ebx = 0x00000000 esi = 0x02a8aab0 edi = 0x00000000 eax = 0x00000000 ecx = 0x00000000 edx = 0x01f80844 efl = 0x00010246 1 xul.dll!mozilla::storage::Statement::~Statement() [mozStorageStatement.cpp:443f7fdb70f7 : 344 + 0xb] eip = 0x0099dbf0 esp = 0x0302fe9c ebp = 0x0302fee8 ebx = 0x00000000 2 xul.dll!mozilla::storage::Statement::Release() [mozStorageStatement.cpp:443f7fdb70f7 : 348 + 0x34] eip = 0x0099dcf5 esp = 0x0302fea8 ebp = 0x0302fee8 3 xul.dll!nsCOMPtr_base::assign_assuming_AddRef(nsISupports *) [nsCOMPtr.h:443f7fdb70f7 : 456 + 0x7] eip = 0x004125b4 esp = 0x0302feb8 ebp = 0x0302fee8 ebx = 0x00000000 4 xul.dll!nsCOMPtr_base::assign_with_AddRef(nsISupports *) [nsCOMPtr.cpp:443f7fdb70f7 : 89 + 0x7] eip = 0x00a63ba1 esp = 0x0302fec8 ebp = 0x0302fee8 5 xul.dll!mozilla::storage::AsyncExecuteStatements::notifyComplete() [mozStorageAsyncStatementExecution.cpp:443f7fdb70f7 : 388 + 0xb] eip = 0x0099e197 esp = 0x0302fed8 ebp = 0x0302fee8 6 xul.dll!mozilla::storage::AsyncExecuteStatements::Run() [mozStorageAsyncStatementExecution.cpp:443f7fdb70f7 : 542 + 0x6] eip = 0x0099e44b esp = 0x0302fef0 ebp = 0x0302fef4 ebx = 0x00000000 7 xul.dll!nsThread::ProcessNextEvent(int,int *) [nsThread.cpp:443f7fdb70f7 : 527 + 0x5] eip = 0x00a818a5 esp = 0x0302fefc ebp = 0x0302ff18 8 xul.dll!NS_ProcessNextEvent_P(nsIThread *,int) [nsThreadUtils.cpp:443f7fdb70f7 : 230 + 0xc] eip = 0x00a6696f esp = 0x0302ff20 ebp = 0x0302ff2c ebx = 0xffffff00 9 xul.dll!nsThread::ThreadFunc(void *) [nsThread.cpp:443f7fdb70f7 : 254 + 0x7] eip = 0x00a81414 esp = 0x0302ff34 ebp = 0x0302ff54 10 nspr4.dll!_PR_NativeRunThread [pruthr.c:443f7fdb70f7 : 426 + 0x8] eip = 0x00f3623b esp = 0x0302ff5c ebp = 0x0302ffb0 ebx = 0x00000001 11 nspr4.dll!pr_root [w95thred.c:443f7fdb70f7 : 122 + 0xc] eip = 0x00f3837d esp = 0x0302ff78 ebp = 0x0302ffb0 ebx = 0x01951568 12 msvcr80.dll + 0x29ba eip = 0x781329bb esp = 0x0302ff80 ebp = 0x0302ffb0 13 msvcr80.dll + 0x2a46 eip = 0x78132a47 esp = 0x0302ffb8 ebp = 0x0302ffec

re-pushed on m-c now that bug 507199 has been fixed. http://hg.mozilla.org/mozilla-central/rev/7515a0b6955b

backed out, now flushes are too late, if a method is adding some bookmark, we endup flushing multiple times when method ends. what we should do instead is having flush as the last observer but this would be hard to do in a non hacky way. http://hg.mozilla.org/mozilla-central/rev/d4c58ffb301c could end up being WONTFIXed

Shawn, is there a way to check at runtime the classID of an added observer (actually a nsISupports*) so that when we add observers we ensure that sync will be the last one (so maybe enqueue sync, push others)? I really would like to see all of these oranges to go away, even if we know the cause they are annoying and creating a lot of useless noise.

(In reply to comment #10) > Shawn, is there a way to check at runtime the classID of an added observer > (actually a nsISupports*) so that when we add observers we ensure that sync > will be the last one (so maybe enqueue sync, push others)? Not that I am aware of, no.

actually i can do that quite easily, just ensuring category observers (atm only dbFlush) being called after usual observers, that's just one macro change. unfortunately this only reduces oranges, but does not solve them. due to this i think the problem could just be subsequent syncs. Btw, i could exchange macro order in bug 478718 if i can solve the last issues with it.

WONTFIXING, i have a workaround in bug 478718 but that demonstrated that i cannot completely solve the issue, at a maximum make it happen less in some case. So would not make much sense.