1125425 - Unbitrot Mark's part 1 patch for getting engines to work with promises/tasks

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Drew Willcoxon :adw]

Broken down from bug 1120695.  Apply Mark's old part 1 patch [1], unbitrot it, and attach the new patch to this bug.  It's hard for me to say whether that's doable in one iteration, or to break it up into smaller parts, without actually starting on it and trying.  I may end up breaking this down further as I go along.

We're not planning on landing things piecemeal on fx-team/m-c, so for now I'll only say that the new patch should be attached to this bug, but we've talked about working together on GitHub or an hg user repo or just a patch queue.  So the result may be to land something there.

[1] https://github.com/mhammond/gecko-dev/commit/d752770f16a2aeea70eaf7a5baeb367881bf1a45

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=144b66f94207

(In reply to Drew Willcoxon :adw from comment #0)
> We're not planning on landing things piecemeal on fx-team/m-c, so for now
> I'll only say that the new patch should be attached to this bug

True in general but we might actually be able to land this first patch on its own.  If we do, Mark points out that we should wait until the start of the next cycle to avoid interfering with FxA/migration and to allow the maximum amount of time for regressions to be worked out.

Comment 2

[Drew Willcoxon :adw]

xpcshell tests failed.  I'll try to see what happened, but I might push off fixing them into another bug if it's hairy.  The point of this bug was just to get the patch unbitrotted.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch 2

My previous patch was bitrotted, which I found when I applied it to investigate the xpcshell test failures.  Not sure if I somehow previously missed the five files that it didn't apply cleanly to, or those files were changed since I last applied the patch.  Anyway, here's another unbitrotted patch (as of fx-team 14abbfddd5c2, Feb 5).

I haven't gotten around to investigating the previous xpcshell errors yet, but here's a new try push with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c6c57c03ff

Comment 4

[Drew Willcoxon :adw]

Attachment: patch 3

Uh, I don't know what's going on, but the try run failed because a test file mentioned in xpcshell.ini was missing.  Even though the patch makes changes to that file.  And sure enough, after I qpopped and qpushed locally, the file was missing for me too.  Luckily it's still in an Emacs buffer.  hg status doesn't show anything after I edit and save the file.

So I hg added the file anyway and then qrefed, qpopped, and qpushed, and now it seems to stick.  No idea.  Here's the resulting patch.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd07f69e4032

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 4

Some of the errors in the first try run in comment 1 were only due to typos.  I fixed those, and here's the updated patch.  Below are the remaining failing tests from that try run that still fail for me locally.  A new try run with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a55873c01c1

services/sync/tests/unit/test_syncengine_sync.js:

>  0:09.07 TEST_STATUS: Thread-1 test_processIncoming_reconcile_locally_deleted_dupe_new FAIL [test_processIncoming_reconcile_locally_deleted_dupe_new : 18] 0 == 1
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/common/tests/unit/head_helpers.js:do_check_attribute_count:18
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/common/tests/unit/head_helpers.js:do_check_empty:14
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_syncengine_sync.js:test_processIncoming_reconcile_locally_deleted_dupe_new:479
>     self-hosted:next:610
>     _run_next_test@/Users/adw/fx/testing/xpcshell/head.js:1376:9
>     do_execute_soon/<.run@/Users/adw/fx/testing/xpcshell/head.js:645:9
>     _do_main@/Users/adw/fx/testing/xpcshell/head.js:208:5
>     _execute_test@/Users/adw/fx/testing/xpcshell/head.js:505:5
>     @-e:1:1

services/sync/tests/unit/test_bookmark_engine.js:

>  0:12.95 TEST_STATUS: Thread-1 test_bookmark_guidMap_fail FAIL [test_bookmark_guidMap_fail : 409] "undefined" == "error.engine.abort.applyincoming"
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_bookmark_engine.js:test_bookmark_guidMap_fail:409
>     self-hosted:throw:627
>     _run_next_test@/Users/adw/fx/testing/xpcshell/head.js:1376:9
>     do_execute_soon/<.run@/Users/adw/fx/testing/xpcshell/head.js:645:9
>     _do_main@/Users/adw/fx/testing/xpcshell/head.js:208:5
>     _execute_test@/Users/adw/fx/testing/xpcshell/head.js:505:5
>     @-e:1:1

services/sync/tests/unit/test_tab_engine.js:

>  0:03.79 TEST_STATUS: Thread-1 test_tab_engine_skips_incoming_local_record FAIL [test_tab_engine_skips_incoming_local_record : 87] Remote client record was applied - 0 == 1
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_tab_engine.js:test_tab_engine_skips_incoming_local_record/engine._syncFinish:87
>     resource://services-sync/engines.js:SyncEngine.prototype._sync<:1543
>     self-hosted:InterpretGeneratorResume:702
>     self-hosted:next:610
>     test_tab_engine_skips_incoming_local_record@/Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_tab_engine.js:95:3
>     _run_next_test@/Users/adw/fx/testing/xpcshell/head.js:1383:11
>     do_execute_soon/<.run@/Users/adw/fx/testing/xpcshell/head.js:645:9
>     _do_main@/Users/adw/fx/testing/xpcshell/head.js:208:5
>     _execute_test@/Users/adw/fx/testing/xpcshell/head.js:505:5
>     @-e:1:1

services/sync/tests/unit/test_clients_engine.js:

>  0:17.28 TEST_STATUS: Thread-1 test_optional_client_fields FAIL [test_optional_client_fields : 577] "undefined" == "adw's Nightly on fourside"
> /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_clients_engine.js:test_optional_client_fields:577
> /Users/adw/fx/testing/xpcshell/head.js:_run_next_test:1383
> /Users/adw/fx/testing/xpcshell/head.js:do_execute_soon/<.run:645
> /Users/adw/fx/testing/xpcshell/head.js:_do_main:208
> /Users/adw/fx/testing/xpcshell/head.js:_execute_test:505
> -e:null:1
> null:null:0

services/sync/tests/unit/test_warn_on_truncated_response.js:

>  0:01.19 TEST_STATUS: Thread-1 test_resource_logs_content_length_mismatch FAIL [test_resource_logs_content_length_mismatch : 49] test that a warning was logged - 0 != 0
> /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_warn_on_truncated_response.js:test_resource_logs_content_length_mismatch:49
> /Users/adw/fx/testing/xpcshell/head.js:_run_next_test:1383
> /Users/adw/fx/testing/xpcshell/head.js:do_execute_soon/<.run:645
> /Users/adw/fx/testing/xpcshell/head.js:_do_main:208
> /Users/adw/fx/testing/xpcshell/head.js:_execute_test:505
> -e:null:1
> null:null:0

services/sync/tests/unit/test_fxa_migration_sentinel.js:

>  0:02.82 TEST_STATUS: Thread-1  FAIL got the sentinel back - null deepEqual {"foo":"bar"}
>     /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_fxa_migration_sentinel.js:null:68
>     self-hosted:next:610
>     _run_next_test@/Users/adw/fx/testing/xpcshell/head.js:1376:9
>     do_execute_soon/<.run@/Users/adw/fx/testing/xpcshell/head.js:645:9
>     _do_main@/Users/adw/fx/testing/xpcshell/head.js:208:5
>     _execute_test@/Users/adw/fx/testing/xpcshell/head.js:505:5
>     @-e:1:1

services/sync/tests/unit/test_fxa_migration.js:

>  0:07.40 TEST_STATUS: Thread-1 testMigration FAIL [testMigration : 238] everything was good while sync was running. - false == true
> /Users/adw/fx/obj-debug/_tests/xpcshell/services/sync/tests/unit/test_fxa_migration.js:testMigration:238
> self-hosted:next:610
> resource://gre/modules/Task.jsm:TaskImpl_run:314
> /Users/adw/fx/testing/xpcshell/head.js:_do_main:208
> /Users/adw/fx/testing/xpcshell/head.js:_execute_test:505
> -e:null:1
> null:null:0

Comment 6

[Drew Willcoxon :adw]

The patch was unbitrotted, so I'm marking this bug complete.  The bug for fixing the test failures is bug 1131363.