Closed Bug 873861 Opened 7 years ago Closed 5 years ago

Intermittent test_addons_store.js | Test timed out, | test failed (with xpcshell return code: 1), see following log:

Categories

(Firefox :: Sync, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED WORKSFORME
mozilla24

People

(Reporter: philor, Unassigned)

References

Details

(Keywords: intermittent-failure, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #795800 +++

Because 'test timed out' is the new 'timed out after 1200 seconds without output'.

https://tbpl.mozilla.org/php/getParsedLog.php?id=23134900&tree=Mozilla-Inbound
Rev3 WINNT 5.1 mozilla-inbound opt test xpcshell on 2013-05-19 09:22:51 PDT for push ec0d43b4533d
slave: talos-r3-xp-115

10:19:13     INFO -  TEST-INFO | C:\talos-slave\test\build\tests\xpcshell\tests\services\sync\tests\unit\test_addons_store.js | running test ...
10:24:13  WARNING -  TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\tests\xpcshell\tests\services\sync\tests\unit\test_addons_store.js | Test timed out
10:24:13     INFO -  Can't trigger Breakpad, just killing process
10:24:13  WARNING -  TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\tests\xpcshell\tests\services\sync\tests\unit\test_addons_store.js | test failed (with xpcshell return code: 1), see following log:
10:24:13     INFO -  >>>>>>>
(a whole bunch of log)
10:24:14     INFO -  TEST-PASS | (xpcshell/head.js) | 99 (+ 0) check(s) passed
10:24:14     INFO -  TEST-INFO | (xpcshell/head.js) | 0 check(s) todo
10:24:14     INFO -  Sync.AddonsReconciler	DEBUG	Stopping listening and removing AddonManager listeners.
10:24:14     INFO -  <<<<<<<

The test was a success but the patient died?
Duplicate of this bug: 873901
Richard, don't suppose you could take a look at this? :-)
Flags: needinfo?(rnewman)
Looking.
Flags: needinfo?(rnewman)
So... the AddonsReconciler spins the event loop inside observer notifications, in order to write out files. That seems suspect.
Attached patch Tentative patch. v1 (obsolete) — Splinter Review
Testing this now, but of course I stand no chance of verifying an intermittent XP failure without just landing the damn thing.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #760514 - Flags: review?(gps)
Comment on attachment 760514 [details] [diff] [review]
Tentative patch. v1

Review of attachment 760514 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/addonsreconciler.js
@@ +393,5 @@
> +      if (this._shouldPersist) {
> +        this.saveState(null, callback);
> +      } else {
> +        callback();
> +      }

I don't think we need this. If you are concerned about spinning during xpcom-shutdown (which is allowed), let's just call stopListening() during a new, final test function (as you've implemented).
Attachment #760514 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #60)

> I don't think we need this. If you are concerned about spinning during
> xpcom-shutdown (which is allowed), let's just call stopListening() during a
> new, final test function (as you've implemented).

I was actually concerned about (a) spinning during shutdown, (b) spinning during observer notifications, (c) a collision between a file write during an observer notification and another during shutdown, (d) file system nonsense on Windows.

But it turns out that we got the failure on try, so this isn't the fix.
Aha! No, the failure moved to test_addons_engine.
This passes try. You might think it's too thorough...
Attachment #760514 - Attachment is obsolete: true
Attachment #760984 - Flags: review?(gps)
Comment on attachment 760984 [details] [diff] [review]
Proposed patch. v2

Review of attachment 760984 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure why you need both the explicit stopListening and the shouldPersist flag. But, it shouldn't hurt.

Please add code comments detailing reasons for the hackery before commit.
Attachment #760984 - Flags: review?(gps) → review+
https://hg.mozilla.org/services/services-central/rev/1544dcd2f78a

(Inbound is closed, so here we go.)
Whiteboard: [fixed in services][qa-]
https://hg.mozilla.org/mozilla-central/rev/1544dcd2f78a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla24
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Did this stop happening, or did we stop starring it?
It stopped happening.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → WORKSFORME
(Well, or someone accidentally or intentionally disabled the test.)
Too much bother to clone it for a new one.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Unassigning bugs that I'm not actively working on.
Assignee: rnewman → nobody
Closing bugs where TBPLbot has previously commented, but have now not been modified for >3 months & do not contain the whiteboard strings for disabled/annotated tests or use the keyword leave-open. Filter on: mass-intermittent-bug-closure-2014-07
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → WORKSFORME
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.