511965 - Delay Places syncs after results updates

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0 (r=sdwilsh from bug 511860)

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.

Comment 1

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/29d0cbd409ad

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Marco Bonardo [:mak]]

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?

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Workaround to get symbols

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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[Marco Bonardo [:mak]]

re-pushed on m-c now that bug 507199 has been fixed.

http://hg.mozilla.org/mozilla-central/rev/7515a0b6955b

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Shawn Wilsher :sdwilsh]

(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.

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Marco Bonardo [:mak]]

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.