511860 - test_results-as-visits.js randomly (but constantly) fails, and then reports a failure in head_queries.js

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

this is most likely another instance of bug 507790, but since it's happening too often i'm going to disable the test.

Comment 1

[Marco Bonardo [:mak]]

disabled test since it was causing perma orange.
http://hg.mozilla.org/mozilla-central/rev/e932e93fc953

Comment 2

[Marco Bonardo [:mak]]

notice this test executed in loops locally was never failing, and failures on Linux and Windows were slightly different, reporting wrong results with duplicates from the temp tables (16 results on Lin, 23 on Win, correct number was 15)

Comment 3

[Marco Bonardo [:mak]]

the main problem appear related to batches, and i can guess a reason, when we fire onEndUpdateBatch we call Refresh on query nodes, that will cause the node to requery the db sync.
But just before that, DBFlush will receive the notification and start syncing.

I think that if we enqueue the sync we would solve most of the random oranges we have due to underlying db changes. Indeed i can locally reproduce bug 509970 at every run, the same way tinderboxes can reproduce this bug at every run, enqueuing the sync makes the test pass.

So, i'd like to try this approach, won't solve the underlying UNION ALL issue, but delaying the sync will allow our results to update most of the times with fixed underlying data.

Will post a patch, i'm just ensuring tests are passing

Comment 4

[Marco Bonardo [:mak]]

Attachment: delay syncs after results updates

I don't see anything bad in doing this, but could be i'm missing something related to storage.
I'm delaying the sync only when we receive direct notifications, that always happen synchronously and sometimes in the middle of our methods (see removeXXXByTimeframe for example), results listen to those, so it would be better to avoid messing up with the db while results are refreshing.
The purpose of this is not to solve the issue that actually has hard solutions, but to reduce the number of times it could happen to really rare events.

If approved, after letting this bake for a run, i'll re-enable the test to see if it is now working as expected.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 395821 [details] [diff] [review]
delay syncs after results updates

r=sdwilsh

We need to start coming up with a plan on how to get rid of temporary tables.

Comment 6

[Marco Bonardo [:mak]]

i'm splitting out the patch to a new bug because this way is hard to track, will forward r+ there.

Comment 7

[Marco Bonardo [:mak]]

moved patch to bug 511965, will push there, then i'll re-enable the test here if everything works correctly.

Comment 9

[Marco Bonardo [:mak]]

nothing i can do till bug 507790 is solved, but i'll stop orange reports in bug 521225

Comment 10

[Dietrich Ayala (:dietrich)]

WINNT 5.2 mozilla-central test everythingelse on 2009/10/14 15:59:05

TEST-UNEXPECTED-FAIL | e:/builds/moz2_slave/mozilla-central-win32-unittest-everythingelse/build/xpcshell/tests/test_places/queries/test_results-as-visit.js | amo == ab moz - See following stack:

Comment 11

[Marco Bonardo [:mak]]

this is interesting...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255647144.1255648815.12624.gz
WINNT 5.2 mozilla-central test opt everythingelse on 2009/10/15 15:52:24  TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-opt-unittest-everythingelse\build\xpcshell\tests\test_places\queries\test_results-as-visit.js | test failed (with xpcshell return code: 0), see following log:

Comment 14

[Marco Bonardo [:mak]]

i fixed a wrong name in the workaround code and that has stopped reporting this orange. The bug is still there though. But that explains the failure in comment 12.

Comment 15

[Daniel Holbert [:dholbert]]

Adding "head_queries.js" to bug summary, per the duplicate bug marked in comment 13.

Just hit an orange on that test:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257281227.1257283792.13458.gz
OS X 10.5.2 mozilla-central test everythingelse on 2009/11/03 12:47:07

Comment 16

[Marco Bonardo [:mak]]

(In reply to comment #15)
> Adding "head_queries.js" to bug summary, per the duplicate bug marked in
> comment 13.

does not make much sense, any test failing in queries folder will fail in head_queries.js...

Comment 17

[Marco Bonardo [:mak]]

indeed the failing test in comment 15 is test_redirectsMode.js and not test_results-as-visits.js

Comment 18

[Marco Bonardo [:mak]]

just to be clear HEAD_XXX are header files included by xpcshell tests, any failure reported in HEAD_XXX files must be reported to the failing xpcshell test.

an header cannot fail by itself.

Comment 19

[Daniel Holbert [:dholbert]]

Oh, sorry - I misunderstood your comment in Bug 522602.

Still, I think it's worth mentioning "head_queries.js" in the bug's summary, for the benefit of log-starrers, because the tinderbox log *does* highlight the failures as two distinct failures (in two distinct tests).

e.g. "random failure in test_results-as-visits.js, followed by head_queries.js"
(and the same for Bug 523578 -- the test_redirectsMode.js failure.)

This would make orange-diagnosis much easier...  Marco, is this ok by you?

Comment 20

[Marco Bonardo [:mak]]

(In reply to comment #19)
> Still, I think it's worth mentioning "head_queries.js" in the bug's summary,
> for the benefit of log-starrers, because the tinderbox log *does* highlight the
> failures as two distinct failures (in two distinct tests).
> 
> e.g. "random failure in test_results-as-visits.js, followed by head_queries.js"
> (and the same for Bug 523578 -- the test_redirectsMode.js failure.)

i have a fear that in such way people will start reporting failures to the first head_queries bug they find with "head_queries.js" in the title, without bothering to look at the fact it's the same failure.

that said, if "followed by" is specified should be clear enough, so that could be fine.
I'm not sure how we can make clear where to report head_xxx failures, since this problem is not only in Places, can be anywhere in our codebase where a test is using some common util in its headers.

finally, this test should not fail anywhere atm, it's disabled on 1.9.2 and workarounded (But running) on trunk.

Comment 21

[Marco Bonardo [:mak]]

cannot happen anymore on trunk after Places branch merge.