Last Comment Bug 1217544 - Implement one-off BackgroundSync API
: Implement one-off BackgroundSync API
Status: ASSIGNED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Service Workers (show other bugs)
: Trunk
: All All
: -- normal with 11 votes (vote)
: ---
Assigned To: Fernando Jiménez Moreno [:ferjm]
:
: Andrew Overholt [:overholt]
Mentors:
https://wicg.github.io/BackgroundSync...
Depends on: ServiceWorkers-compat
Blocks: 1138827 1138829 1260138 1260141 platform-ui-team 1300844 1308447
  Show dependency treegraph
 
Reported: 2015-10-22 12:21 PDT by Andrew Overholt [:overholt]
Modified: 2017-01-05 14:25 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected


Attachments
Part 1: WebIDL (WIP) (11.51 KB, patch)
2016-01-08 06:07 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (WIP) (8.02 KB, patch)
2016-01-08 06:07 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 3: IPC (WIP) (17.35 KB, patch)
2016-01-08 06:08 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 3: IPC (WIP) (21.67 KB, patch)
2016-01-12 11:06 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 3: IPC (WIP) (27.84 KB, patch)
2016-01-27 07:43 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (WIP) (40.95 KB, patch)
2016-01-27 07:44 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (WIP) (9.98 KB, patch)
2016-01-27 09:37 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 3: IPC (WIP) (29.00 KB, patch)
2016-01-27 09:37 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (WIP) (42.27 KB, patch)
2016-01-27 09:38 PST, Fernando Jiménez Moreno [:ferjm]
amarchesini: feedback+
Details | Diff | Splinter Review
Part 1: WebIDL (14.52 KB, patch)
2016-02-19 10:36 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (9.84 KB, patch)
2016-02-19 10:39 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 3: IPC (31.71 KB, patch)
2016-02-19 10:39 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Tests (4.84 KB, patch)
2016-02-19 10:41 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 4: SyncService (WIP) (6.38 KB, patch)
2016-02-19 11:45 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 4: SyncEvent (28.00 KB, patch)
2016-03-06 12:20 PST, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 4: SyncEvent (28.40 KB, patch)
2016-03-16 11:47 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 5: SyncService (13.99 KB, patch)
2016-03-16 11:50 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (WIP) (79.03 KB, patch)
2016-03-16 12:02 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: feedback+
Details | Diff | Splinter Review
High level architecture (30.01 KB, image/png)
2016-03-16 12:10 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details
Part 5: Sync DB (7.68 KB, patch)
2016-03-22 12:23 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 6: SyncService (22.41 KB, patch)
2016-03-22 12:24 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (WIP) (105.06 KB, patch)
2016-03-30 12:34 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (WIP) (105.06 KB, patch)
2016-03-30 12:39 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 1: WebIDL and SyncManager (11.40 KB, patch)
2016-04-20 09:44 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (12.11 KB, patch)
2016-04-20 09:44 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 3: IPC (32.62 KB, patch)
2016-04-20 09:45 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 4: SyncEvent (26.95 KB, patch)
2016-04-20 09:45 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 5: Sync DB (13.19 KB, patch)
2016-04-20 09:47 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 6: SyncService (50.93 KB, patch)
2016-04-20 09:47 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 7: Tests (15.57 KB, patch)
2016-04-20 09:47 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Unified diff (121.55 KB, patch)
2016-04-20 09:48 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Unified diff (214.24 KB, patch)
2016-06-03 11:14 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 1: WebIDL and DOM bindings (12.34 KB, patch)
2016-06-21 01:23 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (12.59 KB, patch)
2016-06-21 01:28 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3: IPC (34.62 KB, patch)
2016-06-21 01:30 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 4: Sync event (27.95 KB, patch)
2016-06-21 01:31 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 5: Storage (178.68 KB, patch)
2016-06-21 01:36 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 6: OnlineObserver (7.64 KB, patch)
2016-06-21 01:37 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 7: BackgroundSyncService (56.26 KB, patch)
2016-06-21 01:38 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review-
Details | Diff | Splinter Review
Part 8: Tests (9.32 KB, patch)
2016-06-21 01:39 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Unified diff (277.67 KB, patch)
2016-06-21 01:41 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 1: WebIDL and DOM bindings (12.43 KB, patch)
2016-08-03 08:54 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (13.45 KB, patch)
2016-08-03 08:55 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3: IPC (34.28 KB, patch)
2016-08-03 08:57 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3.1. : Interdiff with WorkerFeature to WorkerHolder (9.30 KB, patch)
2016-08-03 09:18 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 4: Sync event (24.70 KB, patch)
2016-08-03 09:21 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 6: OnlineObserver (7.78 KB, patch)
2016-08-03 09:21 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 7: BackgroundSyncService (59.18 KB, patch)
2016-08-04 08:21 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review
Part 1: WebIDL and DOM bindings (12.43 KB, patch)
2016-09-06 10:52 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 2: ServiceWorkerRegistration (13.27 KB, patch)
2016-09-06 10:53 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3: IPC (34.20 KB, patch)
2016-09-06 10:54 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 4: Sync event (24.85 KB, patch)
2016-09-06 10:54 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 5: QuotaClient helpers (52.08 KB, patch)
2016-09-06 11:03 PDT, Fernando Jiménez Moreno [:ferjm]
bkelly: feedback+
Details | Diff | Splinter Review
Part 6: Storage (135.12 KB, patch)
2016-09-06 11:07 PDT, Fernando Jiménez Moreno [:ferjm]
bkelly: feedback-
Details | Diff | Splinter Review
Part 7: OnlineObserver (7.73 KB, patch)
2016-09-06 11:08 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Part 8: BackgroundSyncService (62.85 KB, patch)
2016-09-06 11:11 PDT, Fernando Jiménez Moreno [:ferjm]
amarchesini: review+
Details | Diff | Splinter Review
Part 9: Tests (9.31 KB, patch)
2016-09-06 11:12 PDT, Fernando Jiménez Moreno [:ferjm]
ferjmoreno: review+
Details | Diff | Splinter Review
Unified diff (286.86 KB, patch)
2016-09-06 11:14 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Splinter Review

Description User image Andrew Overholt [:overholt] 2015-10-22 12:21:18 PDT
https://github.com/slightlyoff/BackgroundSync/
Comment 1 User image Ben Kelly [:bkelly] 2015-10-22 12:27:16 PDT
I hope you meant after v1.
Comment 2 User image Andrew Overholt [:overholt] 2015-10-22 12:29:33 PDT
(In reply to Ben Kelly [:bkelly] from comment #1)
> I hope you meant after v1.

Oops, yeah.
Comment 3 User image Andrew Sutherland [:asuth] 2015-12-22 10:23:55 PST
What's the relationship between the planned implementation for BackgroundSync and the existing requestsync API in tree?  (https://dxr.mozilla.org/mozilla-central/source/dom/requestsync)
Comment 4 User image Fernando Jiménez Moreno [:ferjm] 2016-01-08 06:06:55 PST
The existing RequestSync API can be considered as precursor of the planned BackgroundSync API. At some point RequestSync (System Messages based and B2G only) should be removed in favor of BackgroundSync (SW based and cross product), but for now they will live together for a while. So far the only planned implementation for the BackgroundSync API is for one-shot sync requests. Periodic sync requests through the BackgroundSync API, which is a more similar functionality to the one offered by the RequestSync API, is still not planned.
Comment 5 User image Fernando Jiménez Moreno [:ferjm] 2016-01-08 06:07:25 PST
Created attachment 8705632 [details] [diff] [review]
Part 1: WebIDL (WIP)
Comment 6 User image Fernando Jiménez Moreno [:ferjm] 2016-01-08 06:07:57 PST
Created attachment 8705633 [details] [diff] [review]
Part 2: ServiceWorkerRegistration (WIP)
Comment 7 User image Fernando Jiménez Moreno [:ferjm] 2016-01-08 06:08:21 PST
Created attachment 8705634 [details] [diff] [review]
Part 3: IPC (WIP)
Comment 8 User image Andrew Sutherland [:asuth] 2016-01-08 12:43:45 PST
Gotcha, so this is a completely separate C++ implementation.  (Which is great!)

The next question is will the initial implementation of BackgroundSync be wakelock/mozAlarms aware?  (Or whatever the mozAlarms HAL-control-driving successor may be, if there is one.)

Since B2G doesn't have service workers yet and desktop ignores these things, it probably doesn't matter for the initial functionality... but I think requestsync was ill-served by having its initial implementation ignore the reality of needing to hold wakelocks.
Comment 9 User image Ben Kelly [:bkelly] 2016-01-08 12:57:35 PST
Are you talking about a wakelock type thing the service worker script can use?  Or that the background-sync implementation needs to have a mechanism for keeping the device awake while performing background-sync?
Comment 10 User image Andrew Sutherland [:asuth] 2016-01-08 13:29:29 PST
I mainly wondered whether wakelock/suspend issues were being dealt with at all for this rev.

Service workers already have a clear lifetime mechanism with waitUntil (a la https://code.google.com/p/chromium/issues/detail?id=437541) on events.  I'd expect a raw wakelock construct would never be exposed since the service worker needs to always be able to justify its continued existence by means of an active request that can be the actual owner of the (implicit) wakelock.  In the ideal case, I suppose BackgroundSync would hold wakelocks in the following cases:

1) From when the timer fires, which ideally would be a mozAlarm (or successor) so the device can wake-up from suspend to process the sync, BackgroundSync presumably needs to hold a wake-lock until the service worker has been activated and has had a chance to register a 'sync' event and have the event dispatched so that it can invoke waitUntil.

2) Any API calls to mutate registrations should probably hold a wakelock or it should be explicitly documented that it's assumed the caller already holds a wakelock and will not release the wakelock until the Promise returned by the API has been resolved.  In the event callers aren't trusted to wait for the promise, the API should probably acquire its own wakelock.  It's not clear to me whether service workers are being super strict about proper API use.

3) When the recurring BackgroundSync API happens which explicitly is not part of this bug, whatever reschedules the timer needs to be covered by a wakelock.
Comment 11 User image Ben Kelly [:bkelly] 2016-01-08 13:34:05 PST
I think it will depend on the underlying platform.  Android facilities may differ from whats available on b2g.
Comment 12 User image Andrew Sutherland [:asuth] 2016-01-08 13:51:13 PST
We already have unified HAL abstractions for wakelocks and wake-up-from-suspend-timers regardless of underlying platform support.  The questions is whether background sync will use the abstractions or defer using them.

(And honestly, I'm not so much asking a question as strongly suggesting that this be addressed as part of the initial implementation.  The rationale being that it was suboptimal that requestsync landed fundamentally broken and that the wakelock logic had to be awkwardly retrofitted when this was realized.  Since the mail app is likely an eventual consumer of this API, it would be reassuring to know that these important concerns are being addressed up front with assertion coverage, etc.  NB: I do appreciate there were the standard b2g time pressures during the development of requestsync.)
Comment 13 User image Ben Kelly [:bkelly] 2016-01-08 13:53:31 PST
Well, on android there may be a different mechanism for being scheduled with other non-firefox background sync stuff.  My impression was the HAL stuff was more designed around what we wanted in b2g and probably doesn't fit that.  I could be wrong, though.
Comment 14 User image Fernando Jiménez Moreno [:ferjm] 2016-01-12 11:06:34 PST
Created attachment 8707035 [details] [diff] [review]
Part 3: IPC (WIP)
Comment 15 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 07:43:59 PST
Created attachment 8712697 [details] [diff] [review]
Part 3: IPC (WIP)
Comment 16 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 07:44:53 PST
Created attachment 8712699 [details] [diff] [review]
Unified diff (WIP)
Comment 17 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 07:54:08 PST
Comment on attachment 8712699 [details] [diff] [review]
Unified diff (WIP)

Andrea, this patch is not doing much yet, but since this is the first time I implement a DOM API entirely in C++ and I am still quite unfamiliar with all the IPC stuff, I would really appreciate some early feedback before I move forward with the implementation. Thanks!
Comment 18 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 09:37:31 PST
Created attachment 8712744 [details] [diff] [review]
Part 2: ServiceWorkerRegistration (WIP)

Now it works on workers.
Comment 19 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 09:37:57 PST
Created attachment 8712745 [details] [diff] [review]
Part 3: IPC (WIP)
Comment 20 User image Fernando Jiménez Moreno [:ferjm] 2016-01-27 09:38:25 PST
Created attachment 8712746 [details] [diff] [review]
Unified diff (WIP)
Comment 21 User image Andrea Marchesini [:baku] 2016-01-28 03:45:15 PST
Comment on attachment 8712746 [details] [diff] [review]
Unified diff (WIP)

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

Looks very good. I guess the next step is to store these requests in some db and then initialize the manager on startup, right?

::: dom/moz.build
@@ +113,5 @@
>      'resourcestats',
>      'manifest',
>      'vr',
>      'newapps',
> +    'sync'

alphabetic order here too?

::: dom/sync/SyncManager.cpp
@@ +33,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  explicit SyncOpRunnable(Promise* aPromise, SyncOp* aOp)

1. no explicit here.
2. instead using forget() in the CTOR, do:

SyncOpRunnable(Promise* aPromise, nsAutoPtr<SyncOp>& aOp)

@@ +36,5 @@
> +
> +  explicit SyncOpRunnable(Promise* aPromise, SyncOp* aOp)
> +    : mPromise(aPromise)
> +    , mOp(aOp)
> +  {

do you want to assert for aPromise and aOp?

@@ +58,5 @@
> +  }
> +
> +  NS_IMETHODIMP Cancel() override
> +  {
> +    mActor = nullptr;

mPromise = nullptr;
mOp = nullptr;

@@ +120,5 @@
> +    return nullptr;
> +  }
> +
> +  nsAutoPtr<SyncOp> op(new SyncOp(aArgs));
> +  RefPtr<SyncOpRunnable> runnable = new SyncOpRunnable(p, op.forget());

no forget() here.

@@ +127,5 @@
> +    runnable->SetActor(mActor);
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Failed to dispatch to the current thread");
> +    }
> +  } else if (!mShuttingDown) {

why do you want to create a SyncOpRunnable if we are shuttingDown? move this check at the beginning of this method an return and do: aRv.Throw(some kind of error).

@@ +149,5 @@
> +  MOZ_ASSERT(aPrincipal);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  PrincipalInfo principalInfo;
> +  nsresult rv = PrincipalToPrincipalInfo(aPrincipal, &principalInfo);

aRv = PrincipalToPrincipalInfo(...)
if (NS_WARN_IF(aRv.Failed()) {
  return nullptr;
}

@@ +177,5 @@
> +SyncManager::SyncManager(nsIGlobalObject* aGlobal,
> +                         const PrincipalInfo& aPrincipalInfo)
> +  : mGlobal(aGlobal)
> +  , mShuttingDown(false)
> +  , mPrincipalInfo(new PrincipalInfo(aPrincipalInfo))

why do you want to allocate it?

@@ +182,5 @@
> +{
> +  // Register this component to PBackground.
> +  PBackgroundChild* actor = BackgroundChild::GetForCurrentThread();
> +  if (actor) {
> +    this->ActorCreated(actor);

remove this->

@@ +192,5 @@
> +    // Observe XPCOM shutdown
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      DebugOnly<nsresult> rv;
> +      rv = obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID,

This object is owned by a window or a worker. Why do we want to delete the actor on xpcom-shutdown?
Should we use inner-window-destroyed and a WorkerFeature instead?

@@ +194,5 @@
> +    if (obs) {
> +      DebugOnly<nsresult> rv;
> +      rv = obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
> +                            false /* ownsWeak */);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

This could fail. Move this (or something else following my comments) in a Init() method and propagate the error code in case something fails.

@@ +278,5 @@
> +    RefPtr<SyncOpRunnable> runnable = mPendingOperations[i].mRunnable;
> +    MOZ_ASSERT(runnable);
> +    runnable->SetActor(mActor);
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Failed to dispatch to the current thread");

mPendingOperations.Clear();

@@ +296,5 @@
> +  if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
> +    mShuttingDown = true;
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);

here you want to connect this SyncManager with the window... or with the worker.

@@ +300,5 @@
> +      obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +    }
> +
> +    if (mActor) {
> +      RefPtr<TeardownRunnable> runnable = new TeardownRunnable(mActor);

you want to call Shutdown() here instead duplicating all this code.

::: dom/sync/SyncManager.h
@@ +100,5 @@
> +
> +  nsAutoPtr<mozilla::ipc::PrincipalInfo> mPrincipalInfo;
> +
> +  struct PendingOperation;
> +  nsTArray<PendingOperation> mPendingOperations;

they are all runnables. What about if you just do:

nsTArray<nsRefPtr<nsRunnable>> mPendingOperations;

::: dom/sync/SyncManagerChild.cpp
@@ +16,5 @@
> +namespace dom {
> +
> +struct SyncManagerChild::PendingRequest final
> +{
> +  PendingRequest(Promise* aPromise)

explicit

@@ +35,5 @@
> +
> +void
> +SyncManagerChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mActorDestroyed = true;

You should clean the pending promises, correct?

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1301,5 @@
>      "SVGZoomAndPan",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "SVGZoomEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "SyncManager", b2g: false},

why b2g false?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +816,5 @@
>  
>    #endif /* ! MOZ_SIMPLEPUSH */
>  }
>  
> +already_AddRefed<SyncManager>

you can also return a SyncManager* and avoid the:

RefPtr<SyncManager> ret = mSyncManager;
return ret.forget();

@@ +1292,5 @@
>  
>    #endif /* ! MOZ_SIMPLEPUSH */
>  }
>  
> +already_AddRefed<SyncManager>

same here.
Comment 22 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 10:35:26 PST
Thanks for the great feedback Andrea! And sorry for the lack of activity during the past weeks. I've been heads down with CD team stuff.
Comment 23 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 10:36:30 PST
Created attachment 8721385 [details] [diff] [review]
Part 1: WebIDL

Updated with Andrea's feedback addressed. I also took the liberty of alphabetically ordering the entire moz.build.
Comment 24 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 10:39:24 PST
Created attachment 8721387 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

Updated with Andrea's feedback.
Comment 25 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 10:39:55 PST
Created attachment 8721388 [details] [diff] [review]
Part 3: IPC

Updated with Andrea's feedback.
Comment 26 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 10:41:59 PST
Created attachment 8721389 [details] [diff] [review]
Tests
Comment 27 User image Fernando Jiménez Moreno [:ferjm] 2016-02-19 11:45:30 PST
Created attachment 8721408 [details] [diff] [review]
Part 4: SyncService (WIP)
Comment 28 User image Fernando Jiménez Moreno [:ferjm] 2016-03-06 12:20:39 PST
Created attachment 8727182 [details] [diff] [review]
Part 4: SyncEvent
Comment 29 User image Fernando Jiménez Moreno [:ferjm] 2016-03-16 11:47:08 PDT
Created attachment 8731368 [details] [diff] [review]
Part 4: SyncEvent
Comment 30 User image Fernando Jiménez Moreno [:ferjm] 2016-03-16 11:50:08 PDT
Created attachment 8731371 [details] [diff] [review]
Part 5: SyncService

With the current set of patches I am able to register from documents and workers and send sync events to sw \o/.

Next steps:

- Offline/Online logic
- Storage
Comment 31 User image Fernando Jiménez Moreno [:ferjm] 2016-03-16 12:02:17 PDT
Created attachment 8731374 [details] [diff] [review]
Unified diff (WIP)

Andrea, I would appreciate another round of feedback if possible. Changes from the last feedback request (mostly on parts 4 and 5):

- Previous feedback addressed.
- Sync event implementation.
- SyncService implementation (WIP).
- IPC plumbing for communication between SyncService and ServiceWorkerManager to be able to send sync events. I'm specially interested on feedback about this part, I am using PBackground for this communication.

Thank you!
Comment 32 User image Fernando Jiménez Moreno [:ferjm] 2016-03-16 12:10:56 PDT
Created attachment 8731377 [details]
High level architecture
Comment 33 User image Fernando Jiménez Moreno [:ferjm] 2016-03-17 00:58:05 PDT
Comment on attachment 8731374 [details] [diff] [review]
Unified diff (WIP)

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

::: dom/sync/SyncManagerParent.cpp
@@ +64,5 @@
> +      nsTArray<nsString> tags;
> +      const SyncGetTagsResponse response(tags);
> +      Unused << SendResponse(aRequestId, response);
> +      break;
> +    }

Andrea, ignore these two case handlers, please. They are just tests.
Comment 34 User image Andrea Marchesini [:baku] 2016-03-18 03:30:01 PDT
Comment on attachment 8731374 [details] [diff] [review]
Unified diff (WIP)

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

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +76,5 @@
>  
>    void onUnregister(in nsIServiceWorkerRegistrationInfo aInfo);
>  };
>  
> +[scriptable, builtinclass, uuid(0577039d-c34e-4f01-afb4-a423ae650e54)]

not needed.

::: dom/moz.build
@@ +6,5 @@
>  
>  JAR_MANIFESTS += ['jar.mn']
>  
>  interfaces = [
> +    'apps',

This is a big patch. Any non-related changes, put them in a separate patch (same bug).

@@ +101,5 @@
>      'settings',
>      'storage',
>      'svg',
> +    'sync',
> +    'system',

Wondering why we don't check the alphabetic ordering here in our mach code. We should. Maybe file a bug, and attach this part of the patch there.

::: dom/sync/SyncIPCTypes.ipdlh
@@ +53,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +

no extra lines here.

::: dom/sync/SyncManager.cpp
@@ +113,5 @@
> +NS_IMPL_ISUPPORTS(TeardownRunnable, nsICancelableRunnable, nsIRunnable)
> +
> +class SyncManagerFeature final : public workers::WorkerFeature
> +{
> +  SyncManager* mManager;

1. MOZ_NON_OWNING_REF
2. plus a comment saying that is the manager that keeps alive this feature.

@@ +118,5 @@
> +
> +public:
> +  explicit SyncManagerFeature(SyncManager* aManager)
> +    : mManager(aManager)
> +  {

MOZ_ASSERT(aManager);

@@ +159,5 @@
> +  }
> +
> +  RefPtr<SyncManager> ref = new SyncManager(aGlobal, principalInfo, aScope);
> +
> +  // Register as observeer for inner-window-destroyed.

ee

@@ +161,5 @@
> +  RefPtr<SyncManager> ref = new SyncManager(aGlobal, principalInfo, aScope);
> +
> +  // Register as observeer for inner-window-destroyed.
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (obs) {

if obs is null, return an error.

@@ +194,5 @@
> +
> +  ref->mWorkerFeature = new SyncManagerFeature(ref);
> +  JSContext* cx = aWorkerPrivate->GetJSContext();
> +  if (NS_WARN_IF(!aWorkerPrivate->AddFeature(cx, ref->mWorkerFeature))) {
> +    NS_WARNING("Failed to register SyncManager worker feature");

no double warnings. You already have NS_WARN_IF

@@ +207,5 @@
> +                         const PrincipalInfo& aPrincipalInfo,
> +                         const nsAString& aScope)
> +  : mInnerID(0)
> +  , mGlobal(aGlobal)
> +  , mWorkerFeature(nullptr)

this is not needed.

@@ +211,5 @@
> +  , mWorkerFeature(nullptr)
> +  , mShuttingDown(false)
> +  , mPrincipalInfo(MakeUnique<PrincipalInfo>(aPrincipalInfo))
> +  , mScope(aScope)
> +{

MOZ_ASSERT(aGlobal);

@@ +212,5 @@
> +  , mShuttingDown(false)
> +  , mPrincipalInfo(MakeUnique<PrincipalInfo>(aPrincipalInfo))
> +  , mScope(aScope)
> +{
> +  // Register this component to PBackground.

Just because this a multi-thread object, store the owning thread in the CTOR (just for debug builds) then, for each method, assert that the code is running in that thread.

@@ +222,5 @@
> +  }
> +}
> +
> +SyncManager::~SyncManager()
> +{

Here for instance, we must be sure that this code runs on the correct thread.

@@ +232,5 @@
> +void
> +SyncManager::Shutdown()
> +{
> +  if (mWorkerFeature) {
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();

MOZ_ASSERT(workerPrivate);
MOZ_ASSERT(workerPrivate->Assert the thread);

@@ +233,5 @@
> +SyncManager::Shutdown()
> +{
> +  if (mWorkerFeature) {
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +    workerPrivate->RemoveFeature(workerPrivate->GetJSContext(), mWorkerFeature);

removeFeature doesn't require JSContext anymore.

@@ +243,5 @@
> +    NS_DispatchToCurrentThread(runnable);
> +    mActor = nullptr;
> +  }
> +
> +  mPendingOperations.Clear();

You must set mShuttingDown=true here.

@@ +292,5 @@
> +  MOZ_ASSERT(aActor);
> +  MOZ_ASSERT(!mActor);
> +
> +  if (mShuttingDown) {
> +    mPendingOperations.Clear();

This should not be needed. How can we have a pending operation here?

@@ +334,5 @@
> +
> +  if (innerID != mInnerID) {
> +    return NS_OK;
> +  }
> +

Call Shutdown() here.

@@ +335,5 @@
> +  if (innerID != mInnerID) {
> +    return NS_OK;
> +  }
> +
> +  mShuttingDown = true;

All of this should be in Shutdown, right?

@@ +357,5 @@
> +already_AddRefed<Promise>
> +SyncManager::ExecuteOp(const SyncOpArgs& aArgs, ErrorResult& aRv)
> +{
> +  if (mShuttingDown) {
> +    mPendingOperations.Clear();

not needed.

@@ +371,5 @@
> +  RefPtr<SyncOpRunnable> runnable = new SyncOpRunnable(p, op);
> +
> +  if (mActor) {
> +    runnable->SetActor(mActor);
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {

what about if here we do:

nsresult rv = NS_DispatchTo...();
if (NS_WARN_IF(NS_FAILED(rv))) {
  p->MaybeReject(rv);
  return;
}

otherwise this promise will be unresolved forever.

@@ +375,5 @@
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Failed to dispatch to the current thread");
> +    }
> +  } else {
> +    mPendingOperations.AppendElement(runnable);

What about if:

if (!mActor) {
  mPendingOperations.AppendElement(runnable);
  return;
}

... all the rest ...

::: dom/sync/SyncManager.h
@@ +62,5 @@
> +  CreateOnWorker(nsIGlobalObject* aGlobal,
> +                 workers::WorkerPrivate* aWorkerPrivate,
> +                 const nsAString& aScope);
> +
> +  uint64_t mInnerID;

this should be private.

::: dom/sync/SyncManagerChild.cpp
@@ +36,5 @@
> +void
> +SyncManagerChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mActorDestroyed = true;
> +  mPendingRequests.Clear();

well.. I would reject all of the pending requests.

@@ +63,5 @@
> +Promise*
> +SyncManagerChild::GetPendingRequest(const nsID& aID)
> +{
> +  PendingRequest* request = mPendingRequests.Get(aID);
> +  if (!request) {

NS_WARN_IF

@@ +94,5 @@
> +SyncManagerChild::RecvResponse(const nsID& aRequestId,
> +                               const SyncOpResponse& aResponse)
> +{
> +  Promise* p = GetPendingRequest(aRequestId);
> +  if (!p) {

1. NS_WARN_IF
2. write a comment saying that this is bad.

@@ +100,5 @@
> +  }
> +
> +  switch(aResponse.type()) {
> +    case SyncOpResponse::TSyncRegisterResponse:
> +    {

you don't need this {} in any of these cases.

@@ +102,5 @@
> +  switch(aResponse.type()) {
> +    case SyncOpResponse::TSyncRegisterResponse:
> +    {
> +      p->MaybeResolve(true);
> +      break;

return true;

@@ +107,5 @@
> +    }
> +    case SyncOpResponse::TSyncGetTagsResponse:
> +    {
> +      p->MaybeResolve(aResponse.get_SyncGetTagsResponse().mTags());
> +      break;

return true;

@@ +113,5 @@
> +    case SyncOpResponse::TSyncOpError:
> +    {
> +      p->MaybeReject(
> +          static_cast<nsresult>(aResponse.get_SyncOpError().mCode()));
> +      break;

return true;

@@ +117,5 @@
> +      break;
> +    }
> +    default:
> +    {
> +      MOZ_CRASH("Unknown BackgroundSync response");

return false;

@@ +120,5 @@
> +    {
> +      MOZ_CRASH("Unknown BackgroundSync response");
> +    }
> +  }
> +  p->MaybeResolve(true);

remove this.

::: dom/sync/SyncManagerChild.h
@@ +41,5 @@
> +  virtual bool RecvResponse(const nsID& aRequestId,
> +                            const SyncOpResponse& aResponse) override;
> +
> +private:
> +  explicit SyncManagerChild();

no explicit.

::: dom/sync/SyncManagerParent.cpp
@@ +48,5 @@
> +
> +  switch(aOp.mArgs().type()) {
> +    case SyncOpArgs::TSyncRegisterArgs:
> +    {
> +      // XXX this needs to be async

Why? this IPC call is already async.

@@ +55,5 @@
> +          aOp.mArgs().get_SyncRegisterArgs().mTag());
> +      const SyncRegisterResponse response(true);
> +      //const SyncOpError response(static_cast<uint32_t>(NS_ERROR_FAILURE));
> +      Unused << SendResponse(aRequestId, response);
> +      break;

return true;

@@ +63,5 @@
> +      //XXX Do GetTags.
> +      nsTArray<nsString> tags;
> +      const SyncGetTagsResponse response(tags);
> +      Unused << SendResponse(aRequestId, response);
> +      break;

return true;

@@ +75,5 @@
> +}
> +
> +bool SyncManagerParent::RecvShutdown()
> +{
> +  AssertIsOnBackgroundThread();

Write a comment in the ipdl file explaining this shutdown method and all the shutting down procedure.

::: dom/sync/SyncManagerParent.h
@@ +36,5 @@
> +  ~SyncManagerParent();
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +
> +  RefPtr<SyncService> mService;

Write a comment here saying that the parent objects keep the service alive.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +1122,5 @@
> +{
> +}
> +
> +already_AddRefed<SyncEvent>
> +SyncEvent::Constructor(mozilla::dom::EventTarget* aOwner,

do you need this mozilla::dom:: ?

::: dom/workers/ServiceWorkerEvents.h
@@ +329,5 @@
> +  nsString mTag;
> +  bool mLastChance;
> +
> +protected:
> +  explicit SyncEvent(mozilla::dom::EventTarget* aOwner);

here too.

@@ +340,5 @@
> +
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx,
> +                                       JS::Handle<JSObject*> aGivenProto) override
> +  {
> +    return mozilla::dom::SyncEventBinding_workers::Wrap(aCx, this, aGivenProto);

here too.
Comment 35 User image Fernando Jiménez Moreno [:ferjm] 2016-03-22 12:23:48 PDT
Created attachment 8733495 [details] [diff] [review]
Part 5: Sync DB
Comment 36 User image Fernando Jiménez Moreno [:ferjm] 2016-03-22 12:24:26 PDT
Created attachment 8733497 [details] [diff] [review]
Part 6: SyncService
Comment 37 User image Fernando Jiménez Moreno [:ferjm] 2016-03-30 12:34:25 PDT
Created attachment 8736435 [details] [diff] [review]
Unified diff (WIP)

This patch addresses your previous feedback and adds:

- SyncService logic to manage sync registrations and trigger sync events when online.
- SyncRegistry
- Several crash fixes.

I still need to deal with one more crash and write more tests, but in the mean time I could use your feedback about the latest changes.

Thank you!
Comment 38 User image Fernando Jiménez Moreno [:ferjm] 2016-03-30 12:39:48 PDT
Created attachment 8736438 [details] [diff] [review]
Unified diff (WIP)

And now with the correct patch. Sorry for the noise.
Comment 39 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:44:26 PDT
Created attachment 8743357 [details] [diff] [review]
Part 1: WebIDL and SyncManager
Comment 40 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:44:56 PDT
Created attachment 8743359 [details] [diff] [review]
Part 2: ServiceWorkerRegistration
Comment 41 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:45:26 PDT
Created attachment 8743360 [details] [diff] [review]
Part 3: IPC
Comment 42 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:45:56 PDT
Created attachment 8743362 [details] [diff] [review]
Part 4: SyncEvent
Comment 43 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:47:02 PDT
Created attachment 8743363 [details] [diff] [review]
Part 5: Sync DB
Comment 44 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:47:25 PDT
Created attachment 8743364 [details] [diff] [review]
Part 6: SyncService
Comment 45 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:47:50 PDT
Created attachment 8743365 [details] [diff] [review]
Part 7: Tests
Comment 46 User image Fernando Jiménez Moreno [:ferjm] 2016-04-20 09:48:19 PDT
Created attachment 8743366 [details] [diff] [review]
Unified diff
Comment 47 User image Andrea Marchesini [:baku] 2016-04-20 12:15:26 PDT
Comment on attachment 8743357 [details] [diff] [review]
Part 1: WebIDL and SyncManager

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

::: dom/sync/SyncManager.cpp
@@ +55,5 @@
> +  aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +  const PrincipalInfo& principalInfo = aWorkerPrivate->GetPrincipalInfo();
> +
> +  RefPtr<SyncManager> ref = new SyncManager(nullptr, principalInfo);

instead passing nullptr, you can do:

new SyncManager(aWorkerPrivate->GlobalScope(), principalInfo);

@@ +86,5 @@
> +
> +  // If we are on the main thread, then check the pref directly.
> +  if (NS_IsMainThread()) {
> +    bool enabled = false;
> +    Preferences::GetBool("dom.background.sync.enabled", &enabled);

return Preferences::GetBool("dom.background.sync.enabled", false);

::: dom/sync/SyncManager.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_SyncManager_h
> +#define mozilla_dom_SyncManager_h
> +
> +#include "nsIPrincipal.h"

forward declaration?

@@ +13,5 @@
> +#include "mozilla/AlreadyAddRefed.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/BindingDeclarations.h"
> +
> +#include "nsCOMPtr.h"

This should go with the rest of the ns* headers.

@@ +53,5 @@
> +  nsIGlobalObject*
> +  GetParentObject() const
> +  {
> +    // Can be nullptr on workers.
> +    return mGlobal;

It should not. nsIGlobalObject is available on workers.
Comment 48 User image Andrea Marchesini [:baku] 2016-04-20 12:31:54 PDT
Comment on attachment 8743359 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

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

::: dom/sync/SyncManager.cpp
@@ +87,5 @@
>  
>    // If we are on the main thread, then check the pref directly.
>    if (NS_IsMainThread()) {
>      bool enabled = false;
> +    Preferences::GetBool("dom.backgroundSync.enabled", &enabled);

same comment of the previous patch.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +1261,5 @@
> +SyncManager*
> +ServiceWorkerRegistrationWorkerThread::GetSync(ErrorResult& aRv)
> +{
> +  if (!mSyncManager) {
> +    mSyncManager = SyncManager::CreateOnWorker(mWorkerPrivate->GlobalScope(),

This is not needed, right? you can get the GlobalScope from mWorkerPrivate.
Comment 49 User image Andrea Marchesini [:baku] 2016-04-21 03:45:02 PDT
Comment on attachment 8743360 [details] [diff] [review]
Part 3: IPC

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

::: dom/sync/SyncManager.cpp
@@ +30,5 @@
>  
> +// Helpers
> +
> +class SyncOpRunnable final : public nsIRunnable,
> +                             public nsICancelableRunnable

class SyncOpRunnable final : public nsCancelableRunnable

@@ +33,5 @@
> +class SyncOpRunnable final : public nsIRunnable,
> +                             public nsICancelableRunnable
> +{
> +public:
> +  NS_DECL_ISUPPORTS

remove this.

@@ +50,5 @@
> +    MOZ_ASSERT(aActor);
> +    mActor = aActor;
> +  }
> +
> +  NS_IMETHODIMP Run() override

NS_IMETHOD

@@ +54,5 @@
> +  NS_IMETHODIMP Run() override
> +  {
> +    MOZ_ASSERT(mActor);
> +    if (mActor->IsActorDestroyed()) {
> +      return NS_OK;

This check is already done in ExecuteOp. Call directly mActor->ExecuteOp(mPromise, mOp);

@@ +60,5 @@
> +
> +    return mActor->ExecuteOp(mPromise, mOp);
> +  }
> +
> +  NS_IMETHODIMP Cancel() override

NS_IMETHOD

@@ +76,5 @@
> +  nsAutoPtr<SyncOp> mOp;
> +  RefPtr<SyncManagerChild> mActor;
> +};
> +
> +NS_IMPL_ISUPPORTS(SyncOpRunnable, nsICancelableRunnable, nsIRunnable)

and remove this.

@@ +79,5 @@
> +
> +NS_IMPL_ISUPPORTS(SyncOpRunnable, nsICancelableRunnable, nsIRunnable)
> +
> +class TeardownRunnable final : public nsIRunnable,
> +                               public nsICancelableRunnable

use nsCancelableRunnable

@@ +82,5 @@
> +class TeardownRunnable final : public nsIRunnable,
> +                               public nsICancelableRunnable
> +{
> +public:
> +  NS_DECL_ISUPPORTS

remove

@@ +90,5 @@
> +  {
> +    MOZ_ASSERT(mActor);
> +  }
> +
> +  NS_IMETHODIMP Run() override

NS_IMETHOD

@@ +94,5 @@
> +  NS_IMETHODIMP Run() override
> +  {
> +    MOZ_ASSERT(mActor);
> +    if (!mActor->IsActorDestroyed()) {
> +      mActor->SendShutdown();

This is OK. But what about if you do: mActor->Shutdown();

and in the actor you implement:

void Shutdown() {
  if (!mActorDestroyed) {
    SendShutdown();
  }
}

then you can remove IsActorDestroyed() completely.

@@ +99,5 @@
> +    }
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHODIMP Cancel() override

NS_IMETHOD

@@ +111,5 @@
> +
> +  RefPtr<SyncManagerChild> mActor;
> +};
> +
> +NS_IMPL_ISUPPORTS(TeardownRunnable, nsICancelableRunnable, nsIRunnable)

remove

@@ +113,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(TeardownRunnable, nsICancelableRunnable, nsIRunnable)
> +
> +class SyncManagerFeature final : public workers::WorkerFeature

remove 'workers::'.

@@ +126,5 @@
> +    MOZ_ASSERT(aManager);
> +    MOZ_COUNT_CTOR(SyncManagerFeature);
> +  }
> +
> +  virtual bool Notify(workers::Status aStatus) override

remove 'workers::'

@@ +137,5 @@
> +
> +private:
> +  ~SyncManagerFeature()
> +  {
> +    MOZ_COUNT_CTOR(SyncManagerFeature);

DTOR

@@ +163,5 @@
>    RefPtr<SyncManager> ref = new SyncManager(aGlobal, principalInfo);
> +
> +  // Register as observer for inner-window-destroyed.
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (obs) {

if (!obs) {
  aRv.Throw(NS_ERROR...);
  return nullptr;
}

obs->AddObserver ...

@@ +232,5 @@
> +  Shutdown();
> +  MOZ_ASSERT(!mWorkerFeature);
> +  MOZ_ASSERT(!mActor);
> +}
> +

#ifdef DEBUG

@@ +236,5 @@
> +
> +bool
> +SyncManager::IsSyncManagerThread() {
> +  return mThread == nsCOMPtr<nsIThread>(do_GetCurrentThread());
> +};

#endif

@@ +326,5 @@
> +    MOZ_ASSERT(runnable);
> +    runnable->SetActor(mActor);
> +    if (NS_FAILED(NS_DispatchToCurrentThread(runnable))) {
> +      NS_WARNING("Failed to dispatch to the current thread");
> +      mPendingOperations.Clear();

remove this, and do: break

::: dom/sync/SyncManagerChild.cpp
@@ +18,5 @@
> +struct SyncManagerChild::PendingRequest final
> +{
> +  explicit PendingRequest(Promise* aPromise)
> +    : mPromise(aPromise)
> +  {}

MOZ_ASSERT(aPromise)

@@ +51,5 @@
> +}
> +
> +nsresult
> +SyncManagerChild::StorePendingRequest(Promise* aPromise, nsID& aID)
> +{

MOZ_ASSERT(aPromise);

@@ +57,5 @@
> +  nsCOMPtr<nsIUUIDGenerator> uuidGenerator =
> +    do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsID id;

You can directly use aID everywhere here, right?

@@ +70,5 @@
> +  return NS_OK;
> +}
> +
> +Promise*
> +SyncManagerChild::GetPendingRequest(const nsID& aID)

Take.. instead Get ?

@@ +85,5 @@
> +}
> +
> +nsresult
> +SyncManagerChild::ExecuteOp(Promise* aPromise, SyncOp* aOp)
> +{

MOZ_ASSERT(aPromise);
MOZ_ASSERT(aOp);

@@ +104,5 @@
> +SyncManagerChild::RecvResponse(const nsID& aRequestId,
> +                               const SyncOpResponse& aResponse)
> +{
> +  Promise* p = GetPendingRequest(aRequestId);
> +  if (!p) {

NS_WARN_IF
can it happen that we don't have this aRequestID?

::: dom/sync/SyncManagerChild.h
@@ +30,5 @@
> +
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(SyncManagerChild)
> +
> +  bool IsActorDestroyed() const

Implementing Shutdown() you can remove this.

::: dom/sync/SyncManagerParent.cpp
@@ +24,5 @@
> +{
> +  AssertIsOnBackgroundThread();
> +}
> +
> +void SyncManagerParent::ActorDestroy(ActorDestroyReason aWhy)

You don't need to implement this if you don't use it.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +485,5 @@
> +BackgroundChildImpl::DeallocPSyncManagerChild(PSyncManagerChild* aActor)
> +{
> +  RefPtr<SyncManagerChild> child =
> +    dont_AddRef(static_cast<SyncManagerChild*>(aActor));
> +  MOZ_ASSERT(child);

MOZ_ASSERT(aActor); and move it before REfPtr<..
Comment 50 User image Andrea Marchesini [:baku] 2016-04-24 20:07:41 PDT
Comment on attachment 8743362 [details] [diff] [review]
Part 4: SyncEvent

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +1157,5 @@
> +{
> +}
> +
> +already_AddRefed<SyncEvent>
> +SyncEvent::Constructor(mozilla::dom::EventTarget* aOwner,

remove mozilla::dom::

::: dom/workers/ServiceWorkerEvents.h
@@ +323,5 @@
> +  nsString mTag;
> +  bool mLastChance;
> +
> +protected:
> +  explicit SyncEvent(mozilla::dom::EventTarget* aOwner);

do you need mozilla::dom:: ?

@@ +354,5 @@
> +    return Constructor(owner, aType, aOptions, aRv);
> +  }
> +
> +  void
> +  GetTag(nsAString& aTag)

const

@@ +360,5 @@
> +    aTag = mTag;
> +  }
> +
> +  bool
> +  LastChance()

const

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1564,5 @@
> +  MOZ_ASSERT(mKeepAliveToken);
> +
> +  nsMainThreadPtrHandle<ServiceWorkerRegistrationInfo> regInfo(
> +      new nsMainThreadPtrHolder<ServiceWorkerRegistrationInfo>(aRegistration, false));
> +

2 lines.
Comment 51 User image Andrea Marchesini [:baku] 2016-04-24 20:17:32 PDT
Comment on attachment 8743363 [details] [diff] [review]
Part 5: Sync DB

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

Before reviewing this I need more informations:

1. This means that nsISyncRegistry can be used only on main-thread. Can we have something that works only on PBackground? (of course this means that we cannot use JS code...)

2. This must speak with QuotaManager because, what about if I do: forget-this-site? Or if QuotaManager decides to delete everything from a particular originAttributes?

::: dom/sync/SyncRegistration.cpp
@@ +31,5 @@
> +}
> +
> +/* attribute DOMString originSuffix; */
> +NS_IMETHODIMP
> +nsSyncRegistration::GetOriginSuffix(nsAString & aOriginSuffix)

remove the space between nsAString and &. everywhere in this file.
Comment 52 User image Fernando Jiménez Moreno [:ferjm] 2016-04-25 01:55:53 PDT
Comment on attachment 8743364 [details] [diff] [review]
Part 6: SyncService

As discussed over IRC, I'll be changing the DB part to use the QuotaManager, form JS to C++ and from IDB to MozStorage, so it can run in I/O thread from PBackground.

For posterity's sake, the reasons to do this:

[10:44:57]  <baku>	1. we don't want to keep the main-thread busy
[10:45:33]  <baku>	2. we don't also want to use the main-thread as a bridge between PBackground and PBackground again
[10:46:32]  <baku>	3. we also want to be fully integrated with QM in order to receive notifications when a domain is deleted
Comment 53 User image Andrea Marchesini [:baku] 2016-05-09 02:25:39 PDT
Comment on attachment 8743365 [details] [diff] [review]
Part 7: Tests

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

::: dom/sync/tests/test_basic.html
@@ +7,5 @@
> +http://creativecommons.org/licenses/publicdomain/
> +
> +-->
> +<head>
> +  <title>Test for Bug 1217544</title>

add a better title.

@@ +18,5 @@
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1217544">Mozilla Bug 1217544</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +
> +</div>

This is because we continue doing copy and paste of existing tests :)
Remove:
<p id="display"></p>
<div id="content" style="display: none">
 
</div>
<pre id="test">
</pre>

::: dom/sync/tests/test_preference.html
@@ +20,5 @@
> +<div id="content" style="display: none">
> +
> +</div>
> +<pre id="test">
> +</pre>

same here.
Comment 54 User image Fernando Jiménez Moreno [:ferjm] 2016-06-03 11:14:02 PDT
Created attachment 8759773 [details] [diff] [review]
Unified diff

Unified diff with the WIP refactor
Comment 55 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:23:33 PDT
Created attachment 8763785 [details] [diff] [review]
Part 1: WebIDL and DOM bindings
Comment 56 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:28:38 PDT
Created attachment 8763788 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

This one has not changed except for the s/Sync/BackgroundSync and the review feedback from comment 48. So keeping the r+.
Comment 57 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:30:11 PDT
Created attachment 8763790 [details] [diff] [review]
Part 3: IPC
Comment 58 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:31:52 PDT
Created attachment 8763791 [details] [diff] [review]
Part 4: Sync event

This one didn't changed much apart from the s/Sync/BackgroundSync change so taking r+ from comment 50.
Comment 59 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:36:14 PDT
Created attachment 8763794 [details] [diff] [review]
Part 5: Storage

This is where most part of the refactor is. As planned in comment 52, we are now using the QuotaManager and MozStorage instead of IDB.

I took a significant part of this code from the Cache API, as we have similar requirements. At some point we may want to abstract and move some of this code to a common helper.
Comment 60 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:37:47 PDT
Created attachment 8763795 [details] [diff] [review]
Part 6: OnlineObserver
Comment 61 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:38:24 PDT
Created attachment 8763796 [details] [diff] [review]
Part 7: BackgroundSyncService
Comment 62 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:39:01 PDT
Created attachment 8763797 [details] [diff] [review]
Part 8: Tests
Comment 63 User image Fernando Jiménez Moreno [:ferjm] 2016-06-21 01:41:41 PDT
Created attachment 8763799 [details] [diff] [review]
Unified diff
Comment 64 User image Andrea Marchesini [:baku] 2016-07-07 01:59:27 PDT
Comment on attachment 8763790 [details] [diff] [review]
Part 3: IPC

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

::: dom/backgroundsync/BackgroundSync.cpp
@@ +92,5 @@
> +  {
> +    MOZ_ASSERT(mActor);
> +  }
> +
> +  NS_IMETHODIMP Run() override

NS_IMETHOD

@@ +101,5 @@
> +    }
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHODIMP Cancel() override

NS_IMETHOD

@@ +115,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(TeardownRunnable, nsICancelableRunnable, nsIRunnable)
> +
> +class BackgroundSyncFeature final : public workers::WorkerFeature

everything is changed \o/
Now it's called WorkerHolder and it does magic things in the DTOR.

@@ +242,5 @@
> +  MOZ_ASSERT(!mActor);
> +}
> +
> +bool
> +BackgroundSync::IsBackgroundSyncThread() {

This method is just used in debug builds. Do:

#ifdef DEBUG
bool
BackgroundSync::IsBackgroundSyncThread() {
 ...
#endif

same in the header file.

::: dom/backgroundsync/BackgroundSyncChild.cpp
@@ +58,5 @@
> +  nsCOMPtr<nsIUUIDGenerator> uuidGenerator =
> +    do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsID id;

rv = nsContentUtils::GenerateUUIDInPlace(id);

::: dom/backgroundsync/BackgroundSyncParent.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +namespace ipc {
> +class BackgroundParentImpl;

In patch 1 you indented like this:

namespace foo {
  class Bar;
}

Change this, or change that.

@@ +46,5 @@
> +} // namespace mozilla
> +
> +#endif // mozilla_dom_BackgroundSyncParent_h
> +
> +

remove this line at the end of the file.
Comment 65 User image Andrea Marchesini [:baku] 2016-07-07 02:00:50 PDT
I gave you r+, but I want to see a interdiff with WorkerFeature to WorkerHolder.
Comment 66 User image Andrea Marchesini [:baku] 2016-07-11 09:25:38 PDT
Comment on attachment 8763795 [details] [diff] [review]
Part 6: OnlineObserver

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

::: dom/backgroundsync/OnlineStateObserver.cpp
@@ +133,5 @@
> +
> +  if (mShuttingDown) {
> +    return NS_OK;
> +  }
> +

Write a comment saying that here we are NS_IOSERVICE_OFFLINE_STATUS_TOPIC

@@ +150,5 @@
> +OnlineStateObserver::Run()
> +{
> +  AssertIsOnMainThread();
> +
> +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();

if (NS_WARN_IF(!os)) {
  return NS_ERROR_FAILURE;
}

@@ +165,5 @@
> +  nsCOMPtr<nsIIOService> ioService = services::GetIOService();
> +  NS_ENSURE_STATE(ioService);
> +  NS_WARN_IF(NS_FAILED(ioService->GetOffline(&offline)));
> +
> +  OnlineState state = UNKNOWN;

What about:

OnlineState state = eUnknown;
if (NS_SUCCEEDED(ioService->GetOffline(&offline))) {
  state = offline ? eOffline : eOnline;
}

::: dom/backgroundsync/OnlineStateObserver.h
@@ +18,5 @@
> +  NS_DECL_NSIRUNNABLE
> +  NS_DECL_NSIOBSERVER
> +
> +  typedef enum {
> +    ONLINE,

eOnline, eOffline and eUnknown
Comment 67 User image Andrea Marchesini [:baku] 2016-07-14 03:14:11 PDT
Comment on attachment 8763796 [details] [diff] [review]
Part 7: BackgroundSyncService

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

::: dom/backgroundsync/BackgroundSyncParent.cpp
@@ +17,5 @@
>  namespace dom {
>  namespace backgroundsync {
>  
> +namespace {
> +  uint64_t sBackgroundSyncManagerParentId = 1;

1. can be be 0. you do ++sBackgroundSyncManagerParentId
2. remove the indentation.

@@ +70,5 @@
>  BackgroundSyncParent::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    AssertIsOnBackgroundThread();
> +
> +  mActorDestroyed = true;

MOZ_ASSERT(!mActorDestroyed);

@@ +74,5 @@
> +  mActorDestroyed = true;
> +
> +  if (mService) {
> +    mService->UnregisterActor(this);
> +  }

1. You should clean the pending request here. right?
2. Set mService to nullptr.

@@ +83,5 @@
>                                       const SyncOp& aOp)
>  {
>    AssertIsOnBackgroundThread();
> +  
> +  if (NS_WARN_IF(!mService)) {

Remove thie extra space.

@@ +84,5 @@
>  {
>    AssertIsOnBackgroundThread();
> +  
> +  if (NS_WARN_IF(!mService)) {
> +    return false;

Can it be that mServcie is null?!?
MOZ_ASSERT(mService);

@@ +90,2 @@
>  
> +  mService->Request(mId, mStorageManagerId, aRequestId, aOp);

return mService->Request(..)

@@ +112,5 @@
>  
>  bool
>  BackgroundSyncParent::RecvShutdown()
>  {
>    AssertIsOnBackgroundThread();

MOZ_ASSERT(m!ActorDestroyed);

@@ +114,5 @@
>  BackgroundSyncParent::RecvShutdown()
>  {
>    AssertIsOnBackgroundThread();
>  
> +  if (NS_WARN_IF(!mService)) {

This cannot happen.
MOZ_ASSERT(mService);

@@ +119,5 @@
> +    return false;
> +  }
> +
> +  mService->UnregisterActor(this);
> +  mService = nullptr;

Implement a Shutdown() method where you do:

1. mService->Unregister()
2. mService = nullptr
3. mPendingRequests.Clear();
4. mActorDestroyed = true;

@@ +121,5 @@
> +
> +  mService->UnregisterActor(this);
> +  mService = nullptr;
> +
> +  if (!mActorDestroyed) {

This also cannot happen.
Just call Send__delete__

@@ +130,5 @@
>  }
>  
>  void
>  BackgroundSyncParent::OnStorageManagerIdCreated(
> +    StorageManagerId* aManagerId, StorageManagerIdFactory* aFactory,

funny indentation :)

What about a simple:

BackgroundSyncParent::OnStorageManagerIdCreated(StorageManagerId* aManagerId,
                                                StorageManagerIdFactory* aFactory,
                                                const SyncInternalOp& aUnused)

(use a monospace font to see what I meant)

::: dom/backgroundsync/BackgroundSyncService.cpp
@@ +27,5 @@
> +namespace dom {
> +namespace backgroundsync {
> +
> +namespace {
> +  BackgroundSyncService* sInstance = nullptr;

here the indentation.

@@ +39,5 @@
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  GetStorageManagerRunnable(BackgroundSyncService* aService,
> +                            const nsString& aOrigins,

aOrigins? aOrigin... why plural?

@@ +59,5 @@
> +    , mOp(aOp)
> +  {
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +72,5 @@
> +
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsIScriptSecurityManager *securityManager =
> +      nsContentUtils::GetSecurityManager();

This can fail if we are shutting down.

@@ +96,5 @@
> +      rv = mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +    }

I don't understand all of this. I mean... you ignore all the origin except the last one. Why? :)

@@ +121,5 @@
> +NS_IMPL_ISUPPORTS(GetStorageManagerRunnable, nsICancelableRunnable, nsIRunnable)
> +
> +// ----------------------------------------------------------------------------
> +
> +struct BackgroundSyncService::StorageManagerRef final

can this be a class? and make everything public.

@@ +123,5 @@
> +// ----------------------------------------------------------------------------
> +
> +struct BackgroundSyncService::StorageManagerRef final
> +{
> +  explicit StorageManagerRef(const uint64_t aActorId,

no explicit if more than 1 argument.

@@ +155,5 @@
> +    return 0;
> +  }
> +  nsAutoPtr<StorageManagerRef> doomed;
> +  StorageManagerRef* ref;
> +  mStorageManagers.Get(aRequestId, &ref);

Instead doing this Contains + Get, just use Get.

if (!mStorageManagers.Get(aRequestId, &ref)) {
  return 0;
}

Plus, have you checked if you can do:

StorageManagerRef* ref = mStorageManages.Get(aRequestId);
if (!ref) {
  return 0;
}

@@ +157,5 @@
> +  nsAutoPtr<StorageManagerRef> doomed;
> +  StorageManagerRef* ref;
> +  mStorageManagers.Get(aRequestId, &ref);
> +  uint64_t actorId = ref->mActorId;
> +  mStorageManagers.RemoveAndForget(aRequestId, doomed);

Hold on :) Contains + Get + RemoveAndForget seems a bit too much :)

Whawt about this:
nsAutoPtr<StorageManagerRef> ref;
mStorageManagers.RemoveAndForget(aRequestId, ref);
if (!ref) {
  return 0;
}

return ref->mActorId;

@@ +167,5 @@
> +{
> +  AssertIsOnBackgroundThread();
> +
> +  for (auto iter = mStorageManagers.Iter(); !iter.Done(); iter.Next()) {
> +    StorageManagerRef* ref;

StorageManagerRef* ref = iter.UserData();

@@ +169,5 @@
> +
> +  for (auto iter = mStorageManagers.Iter(); !iter.Done(); iter.Next()) {
> +    StorageManagerRef* ref;
> +    mStorageManagers.Get(iter.Key(), &ref);
> +    if (NS_WARN_IF(!ref)) {

remove this.

@@ +175,5 @@
> +    }
> +    if (ref->mActorId == aActorId) {
> +      ref->mStorageManager = nullptr;
> +      nsAutoPtr<StorageManagerRef> doomed;
> +      mStorageManagers.RemoveAndForget(iter.Key(), doomed);

break;

@@ +183,5 @@
> +
> +// ----------------------------------------------------------------------------
> +// ChromeStorageManager
> +
> +struct BackgroundSyncService::ChromeStorageManagerRef final

class + public

@@ +200,5 @@
> +  AssertIsOnBackgroundThread();
> +
> +  ChromeStorageManagerRef* ref =
> +    new ChromeStorageManagerRef(aChromeStorageManager);
> +  mChromeStorageManagers.Put(aRequestId, ref);

I don't like this. Use nsRefPtrHashtable for mChromeStorageManager and it will keep your ChromeStorageManager alive without having ChromeStorageManagerRef.

@@ +214,5 @@
> +  }
> +  ChromeStorageManagerRef* ref;
> +  mChromeStorageManagers.Get(aRequestId, &ref);
> +  nsAutoPtr<ChromeStorageManagerRef> doomed;
> +  mChromeStorageManagers.RemoveAndForget(aRequestId, doomed);

if you use nsRefPtrHashtable, this will be: mChromeStorageManagers.Remove(aRequestId);

@@ +224,5 @@
> +  : mOnlineState(OnlineStateObserver::UNKNOWN)
> +{
> +  AssertIsOnBackgroundThread();
> +
> +  // sInstance is a raw BackgroundSyncService*.

... because the object is kept alive by...

@@ +282,5 @@
> +  MOZ_ASSERT(aParent);
> +  MOZ_ASSERT(mBackgroundSyncActors.Contains(aParent));
> +
> +  ReleaseStorageManagerRef(aParent->Id());
> +  mBackgroundSyncActors.RemoveEntry(aParent);

Let's control the shutdown... What about if you do here:

MaybeShutdown();

And in Maybeshutdown you do:

if (mBackgroundSyncActors.Count() == 0 &&
    mStorageManagers.Count() == 0) &&
    mChromeStorageManagers.Count() == 0) {
  mOnlineStateObserver->Shutdown(this);
  mOnlineStateObserver = nullptr;
}

and in the DTOR you also check: MOZ_ASSERT(!mOnlineStateObserver);

@@ +304,5 @@
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(aParent);
> +  MOZ_ASSERT(mServiceWorkerManagerActors.Contains(aParent));
> +
> +  mServiceWorkerManagerActors.RemoveEntry(aParent);

MaybeShutdown();

@@ +310,5 @@
> +
> +// --------------------------------------------------------------------------
> +
> +void
> +BackgroundSyncService::Request(const uint64_t aActorId,

this should return a bool

@@ +322,5 @@
> +  nsresult rv = StorageManager::GetOrCreate(this,
> +                                            aManagerId,
> +                                            getter_AddRefs(manager));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return Response(aActorId, aRequestId,

also Response should return a boolean

@@ +328,5 @@
> +  }
> +
> +  AddStorageManagerRef(aRequestId, aActorId, manager);
> +
> +  manager->ExecuteRequest(aRequestId, aOp);

This should be return manager->...

@@ +331,5 @@
> +
> +  manager->ExecuteRequest(aRequestId, aOp);
> +}
> +
> +void

boolean

@@ +338,5 @@
> +                                const SyncOpResponse& aResponse)
> +{
> +  AssertIsOnBackgroundThread();
> +
> +  for (auto iter = mBackgroundSyncActors.Iter(); !iter.Done(); iter.Next()) {

It seems that we have to find a better data struct. Iterating in an hashtable is not fast.

@@ +341,5 @@
> +
> +  for (auto iter = mBackgroundSyncActors.Iter(); !iter.Done(); iter.Next()) {
> +    RefPtr<BackgroundSyncParent> parent = iter.Get()->GetKey();
> +    MOZ_ASSERT(parent);
> +    if (parent->Id() != aActorId || parent->ActorDestroyed()) {

well, let's optmize:

if (parent->Id() != aActorId) {
  continue;
}

if (!parent->ActorDestroyed()) {
  return parent->NotifyResponse(...);
}

return true;
}

@@ +345,5 @@
> +    if (parent->Id() != aActorId || parent->ActorDestroyed()) {
> +      continue;
> +    }
> +    parent->NotifyResponse(aRequestId, aResponse);
> +  }

interesting... what should be the correct return value? Can it be that we don't have a valid response?

@@ +350,5 @@
> +}
> +
> +void
> +BackgroundSyncService::OnRequestComplete(const nsID& aRequestId,
> +                                         const SyncOpResponse& aResponse)

I need some comment about the full workflow here. Can you write a huge comment here? or in the .h file?

@@ +375,5 @@
> +      }
> +      break;
> +    case SyncOpResponse::TSyncRemoveResponse:
> +      {
> +        nsString origin = aResponse.get_SyncRemoveResponse().mOrigin();

nsString& origin =

@@ +401,5 @@
> +
> +  uint64_t actorId = ReleaseStorageManagerRef(aRequestId);
> +  // If we have an actor ID, this is a DOM request which response needs to be
> +  // reported back to the content process through the IPC actor.
> +  if (actorId != 0) {

if (actorId) {

@@ +550,5 @@
> + */
> +
> +void
> +BackgroundSyncService::OnOnlineStateChanged(
> +    OnlineStateObserver::OnlineState aState)

indentation

@@ +566,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIUUIDGenerator> uuidGenerator =
> +    do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

nsContentUtils has something to avoid all of this.

::: dom/backgroundsync/ChromeStorageManager.cpp
@@ +305,2 @@
>  {
>    AssertIsOnBackgroundThread();

MOZ_ASSERT(aService);
Comment 68 User image Andrea Marchesini [:baku] 2016-07-14 03:15:54 PDT
Comment on attachment 8763797 [details] [diff] [review]
Part 8: Tests

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

::: dom/backgroundsync/tests/test_utils.js
@@ +1,2 @@
> +function debug(str) {
> +  console.log(str + "\n");

remove "\n"
Comment 69 User image Andrea Marchesini [:baku] 2016-07-14 03:22:03 PDT
Comment on attachment 8763794 [details] [diff] [review]
Part 5: Storage

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

Andrew, I didn't finish the review yet, but can you please help me with the DB+MozStorage part?

::: dom/backgroundsync/BackgroundSync.cpp
@@ +51,5 @@
>      MOZ_ASSERT(aActor);
>      mActor = aActor;
>    }
>  
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +62,5 @@
>  
>      return mActor->ExecuteOp(mPromise, mOp);
>    }
>  
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +95,5 @@
> +public:
> +  RegisterResultRunnable(WorkerPrivate* aWorkerPrivate,
> +                         PromiseWorkerProxy* aProxy,
> +                         nsresult aStatus)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)

WorkerThreadModifyBusyCount is not the default value. Don't use it anymore. Update all your patches.

@@ +135,5 @@
> +    , mPromiseProxy(aPromiseProxy)
> +    , mPrincipalInfo(aPrincipalInfo)
> +    , mScope(aScope)
> +    , mTag(aTag)
> +    , mInitiatingThread(do_GetCurrentThread())

call it backgroundThread.

@@ +136,5 @@
> +    , mPrincipalInfo(aPrincipalInfo)
> +    , mScope(aScope)
> +    , mTag(aTag)
> +    , mInitiatingThread(do_GetCurrentThread())
> +  {}

add some assertions here about aTarget, aPromiseProxy

@@ +138,5 @@
> +    , mTag(aTag)
> +    , mInitiatingThread(do_GetCurrentThread())
> +  {}
> +
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +139,5 @@
> +    , mInitiatingThread(do_GetCurrentThread())
> +  {}
> +
> +  NS_IMETHODIMP
> +  Run() override

Write a comment about that this Run is called twice and why.

@@ +183,5 @@
> +
> +    return rv;
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +193,5 @@
> +private:
> +  ~RegisterRunnable() {}
> +
> +  void
> +  MaybeReject(nsresult aRv)

This method is called only on the main-thread. Assert this.

@@ +541,5 @@
> +  if (!mActor) {
> +    mPendingOperations.AppendElement(aRunnable);
> +    return NS_OK;
> +  }
> +

MOZ_ASSERT(mPendingOperations.IsEmpty())

@@ +565,5 @@
> +  if (!mActor) {
> +    mPendingOperations.AppendElement(runnable);
> +    return;
> +  }
> +

MOZ_ASSERT(mPendingOperations.IsEmpty());

@@ +640,5 @@
> +
> +  const SyncRegisterArgs args(NS_ConvertUTF8toUTF16(origin),
> +                              NS_ConvertUTF8toUTF16(originSuffix),
> +                              mScope, (nsString(aTag)));
> +  ExecuteOp(SyncOpArgs(args), p, aRv);

All of this seems a lot of duplicate code. What about if you rename RegisterRunnable to:

RegisterHelper and you create a Run() method that does:

if (NS_IsMainThread()) {
  Execute(); // where you do all the GetOrigin, ... ExecuteOp()...
} else {
  DispatchToMainThread();
}

So that all this GetOrigin and so on are in 1 single method.
Comment 70 User image Andrew Sutherland [:asuth] 2016-07-14 12:44:15 PDT
Comment on attachment 8763794 [details] [diff] [review]
Part 5: Storage

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #59)
> Created attachment 8763794 [details] [diff] [review]
> Part 5: Storage
>
> I took a significant part of this code from the Cache API, as we have
> similar requirements. At some point we may want to abstract and move some of
> this code to a common helper.

So, indeed there is a very large amount of code that's verbatim or lightly adapted from dom/cache.  Specifically, mapping from dom/cache/* to dom/backgroundsync/*:
- Action => StorageAction
- DBAction same
- DBConnection same
- DBSchema => DBSchema (domain specific, not reusable), DBSchemaUtils (largey reusable modulo the parameterization)
- QuotaClient same (although whitelisted path file names vary)
- Context => StorageContext

Conceptually, I'm okay with refactoring in a follow-up, but I'm not crazy about the idea of reviewing duplicated code.  The options are to do a diff pass and rubber-stamp everything that's basically unchanged from dom/cache, or try and go through things and risk creating diverging code-bases that are more of a nightmare to refactor later on.

If we were going to do the extraction now, I think this is what would make sense:
- Move into mozStorage:
  - DBSchemaUtils: These helpers are concerned with creation/validation of the schema, connection establishment, and incremental vacuum.  A lot of decisions about database configuration (WAL/page/growth parameters) are also baked in.  The DEBUG-only "Expect" metadata classes used for validation fully characterize the schema such that we could also use them for creating the schema too.  (And provide an enhancement path to simple table schema changes like column addition/removal.)
  - DBConnection: This is just a manual wrapper that decorates incremental vacuum on close.  (With the specific "should I vacuum?" being part of DBSchemaUtils.)
- Refactor into a common dom/ place like dom/common, dom/shared, dom/quota/clients or something.
  - {Storage}Action, DBAction
  - QuotaClient: needs the filenames/directories parameterized via subclassing/templating/something.
  - {Storage}Context

:bkelly, thoughts on when it's best to do the extraction and/or how such an extraction out of dom/cache should happen?
Comment 71 User image Ben Kelly [:bkelly] 2016-07-15 14:24:43 PDT
My intention when I wrong dom/cache was to break out these bits.  For example, I tried really hard to make Context something that could be abstracted.  Through the course of reviews some of that abstraction got removed so its not as easy to do as I would like now.

Fernando, if you're up for it I think it really would be good to factor out the common bits here.  I'm less concerned about the SQL/DB stuff, but it would be nice to share the Context/QuotaClient/Action bits.

At the very least I think we should write a follow-up bug to do this and add comments in the code to that effect.

What do you think?
Comment 72 User image Andrew Sutherland [:asuth] 2016-07-20 10:36:15 PDT
Comment on attachment 8763794 [details] [diff] [review]
Part 5: Storage

(Clearing review for now pending outcome of bkelly's needinfo on potential refactoring and given :ferjm is on PTO)
Comment 73 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 08:15:43 PDT
I'm back! :)

Thank you for all the feedback and reviews.

(In reply to Ben Kelly [:bkelly] from comment #71)
> My intention when I wrong dom/cache was to break out these bits.  For
> example, I tried really hard to make Context something that could be
> abstracted.  Through the course of reviews some of that abstraction got
> removed so its not as easy to do as I would like now.
> 
> Fernando, if you're up for it I think it really would be good to factor out
> the common bits here.  I'm less concerned about the SQL/DB stuff, but it
> would be nice to share the Context/QuotaClient/Action bits.
> 
> At the very least I think we should write a follow-up bug to do this and add
> comments in the code to that effect.
> 
> What do you think?

I totally agree that factoring out the common parts is something that we have to do. However, given the limited time that I currently have to work on this, I am worried about delaying this API much longer :\ Specially if I have to adapt the dom/cache part as well.

If a longer delay is acceptable, I can work on this refactor on this bug.
Comment 74 User image Ben Kelly [:bkelly] 2016-08-03 08:20:31 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #73)
> If a longer delay is acceptable, I can work on this refactor on this bug.

Personally I'm willing to accept longer delay to get less technical debt.  If its going to be 3+ months of delay, though, maybe we should re-think a bit.  Maybe try and see how difficult it is?

Thanks!  And welcome back!
Comment 75 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 08:49:03 PDT
Yes, I'll give it a try. For what I saw so far, it shouldn't be as long as 3+ months. I might need to adapt the dom/cache part in a follow-up bug though.
Comment 76 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 08:54:14 PDT
Created attachment 8777369 [details] [diff] [review]
Part 1: WebIDL and DOM bindings

Rebased on top of latest m-i. Keeping baku's r+ after comment 63
Comment 77 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 08:55:11 PDT
Created attachment 8777370 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

Rebased as well. r+ from comment 48.
Comment 78 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 08:57:30 PDT
Created attachment 8777373 [details] [diff] [review]
Part 3: IPC

Addressed Andrea's feedback from comment 64. Interdiff with WorkerFeature to WorkerHolder on the way.
Comment 79 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 09:18:35 PDT
Created attachment 8777386 [details] [diff] [review]
Part 3.1. : Interdiff with WorkerFeature to WorkerHolder

Interdiff with WorkerFeature to WorkerHolder as requested in comment 65
Comment 80 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 09:21:01 PDT
Created attachment 8777388 [details] [diff] [review]
Part 4: Sync event

Rebased. r+ from comment 50
Comment 81 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 09:21:52 PDT
Created attachment 8777390 [details] [diff] [review]
Part 6: OnlineObserver

Addressed Andrea's feedback. r+ from comment 66.
Comment 82 User image Fernando Jiménez Moreno [:ferjm] 2016-08-03 09:22:55 PDT
Comment on attachment 8777388 [details] [diff] [review]
Part 4: Sync event

r+ from comment 50 (now for real)
Comment 83 User image Fernando Jiménez Moreno [:ferjm] 2016-08-04 08:21:33 PDT
Created attachment 8777844 [details] [diff] [review]
Part 7: BackgroundSyncService

Feedback from comment 67 addressed
Comment 84 User image Fernando Jiménez Moreno [:ferjm] 2016-08-04 08:26:59 PDT
(In reply to Andrea Marchesini (baku - PTO 1/8 - 12/8) from comment #67)
> 
> @@ +96,5 @@
> > +      rv = mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL);
> > +      if (NS_WARN_IF(NS_FAILED(rv))) {
> > +        return rv;
> > +      }
> > +    }
> 
> I don't understand all of this. I mean... you ignore all the origin except
> the last one. Why? :)
> 

I think I don't do that :) I get the StorageManager for each origin, not just the one for the last origin. There are two cases here though: for the GetAll operation we have multiple origins, for the rest of operations we have a single origin. I added a comment about that.
Comment 85 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 10:52:23 PDT
Created attachment 8788555 [details] [diff] [review]
Part 1: WebIDL and DOM bindings

Rebased on top of m-i.

r+ from comment 63.
Comment 86 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 10:53:10 PDT
Created attachment 8788558 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

r+ from comment 48.
Comment 87 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 10:54:00 PDT
Created attachment 8788560 [details] [diff] [review]
Part 3: IPC

r+ from comment 64
Comment 88 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 10:54:49 PDT
Created attachment 8788562 [details] [diff] [review]
Part 4: Sync event

r+ from comment 50.
Comment 89 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:03:38 PDT
Created attachment 8788567 [details] [diff] [review]
Part 5: QuotaClient helpers

This patch takes dom/cache/ Context* and Action* to dom/quota/, renaming them as ClientContext* and ClientAction* where "Client" refers to a QuotaManager client. Since that's in essence what they offer: doing all the QM initialization work so storage actions can happen.

The main change is the addition of a "Listener" interface that dom/cache/Manager and dom/backgroundSync/StorageManager must use.

In this patch I won't change the dom/cache implementation as I'd like to do that in a different bug, so this one can land independently.
Comment 90 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:07:36 PDT
Created attachment 8788569 [details] [diff] [review]
Part 6: Storage

This patch implements the storage related pieces of backgroundSync. It makes use of the QuotaClient helpers from the previous patch.

For this API we store a list of sync registrations per each origin and a list of the origins that has any sync registration stored. The former is handled by StorageManager*, the later by ChromeStorageManager*.
Comment 91 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:08:27 PDT
Created attachment 8788571 [details] [diff] [review]
Part 7: OnlineObserver

r+ from comment 66.
Comment 92 User image Ben Kelly [:bkelly] 2016-09-06 11:08:53 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #89)
> In this patch I won't change the dom/cache implementation as I'd like to do
> that in a different bug, so this one can land independently.

I'll try to look at this tomorrow, but I think it would be useful to try to land the cache change at the same time.  It can be in a separate patch here or a different bug.  Having the cache API tests exercising this code would give us a lot of test coverage right out of the gate.
Comment 93 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:11:36 PDT
Created attachment 8788574 [details] [diff] [review]
Part 8: BackgroundSyncService

Rebased and addresses feedback from comment 67
Comment 94 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:12:54 PDT
Created attachment 8788575 [details] [diff] [review]
Part 9: Tests

r+ from comment 68.
Comment 95 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:14:19 PDT
Created attachment 8788576 [details] [diff] [review]
Unified diff
Comment 96 User image Fernando Jiménez Moreno [:ferjm] 2016-09-06 11:16:34 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #89)
> 
> In this patch I won't change the dom/cache implementation as I'd like to do
> that in a different bug, so this one can land independently.

I filed bug 1300844 for that.
Comment 97 User image Ben Kelly [:bkelly] 2016-09-12 08:25:12 PDT
Comment on attachment 8788567 [details] [diff] [review]
Part 5: QuotaClient helpers

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

Overall looks reasonable.  I assume most of this was renaming and there were no significant changes from dom/cache.  If there are non-renaming changes, then please seperate those into different patches.

Jan, are you ok with moving these classes from dom/cache into dom/quota?  We have multiple consumers that want to build on them now.

::: dom/cache/Context.cpp
@@ +466,5 @@
>  
>        mData = nullptr;
>  
>        // If the database was opened, then we should always succeed when creating
> +      // the marker file. If it wasn't opened successfully, then no need to

This seems an unnecessary change.  And I personally tend to write with 2 spaces between sentences.

::: dom/quota/ClientContext.h
@@ +229,5 @@
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(ClientContext)
> +};
> +
> +class FileUtils

None of these routines should be used outside the ClientContext.  Just put all of this in an anonymous namespace in ClientContext.cpp.

If you want consumers of ClientContext to detect previous sessions then provide a mechanism on ClientContext itself.
Comment 98 User image Ben Kelly [:bkelly] 2016-09-12 09:26:53 PDT
Comment on attachment 8788569 [details] [diff] [review]
Part 6: Storage

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

A good start, but I think there are some issues to fix.  In particular, I think we should avoid having separate content and chrome paths for the Manager and database.  Also, the database schema does not seem to handle DOMString values correctly.

We should also add tests that verify this all handles quota shutdown correctly.  See things like this dom/cache test:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_restart.html

::: dom/backgroundsync/BackgroundSyncParent.cpp
@@ +39,5 @@
> +private:
> +  ~PendingRequest() {}
> +
> +  nsID mRequestId;
> +  SyncOp mOp;

Can these be const?

@@ +68,4 @@
>  {
>    AssertIsOnBackgroundThread();
>  
> +  // XXX

What is going to go here?

::: dom/backgroundsync/BackgroundSyncParent.h
@@ +41,5 @@
>    void NotifyResponse(const nsID& aRequestId,
>                        const SyncOpResponse& aResponse);
>  
>  private:
> +  BackgroundSyncParent(const PrincipalInfo& aPrincipalInfo);

explicit

::: dom/backgroundsync/BackgroundSyncTypes.h
@@ +20,5 @@
> +{
> +  PENDING = 1,
> +  WAITING,
> +  FIRING,
> +  REREGISTERING_WHILE_FIRING

Might be nice to have a NUM_STATES or some such value to indicate the limit on the enum.

::: dom/backgroundsync/ChromeStorageManager.h
@@ +27,5 @@
> +
> +class ChromeStorageAction;
> +class Registration;
> +
> +class ChromeStorageManager final

Why have a separate manager and database for chrome things?  The way we handle this in dom/cache is with a "namespace" value:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#158
https://dxr.mozilla.org/mozilla-central/source/dom/cache/Types.h#22

This indicates either content or chrome.

I think this would be preferable to having separate database and managers.

::: dom/backgroundsync/DBConnection.h
@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace backgroundsync {
> +
> +class DBConnection final : public mozIStorageConnection

Rather than duplicate this code exactly in dom/cache and bgsync, I think we should move it into mozstorage.  We could either have a callback mechanism or just call it "IncrementalVacuumConnection" or something.

::: dom/backgroundsync/DBSchema.cpp
@@ +28,5 @@
> +
> +// ---------
> +const char* const kTableRegistrations =
> +  "CREATE TABLE registrations ("
> +    "id TEXT NOT NULL PRIMARY KEY, " // Concatenation of scope + tag

Is the scope a URL?  These can be quite large and we try not to index them.  Instead we usually hash the URL and index the hash instead.  Since you already store the scope in a separate column, this would probably be preferable.  You could even hash the scope and tag together here.

If you do want to keep scope+tag as the primary key, you can do this without duplicating the data in the table.  For example:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#161

@@ +30,5 @@
> +const char* const kTableRegistrations =
> +  "CREATE TABLE registrations ("
> +    "id TEXT NOT NULL PRIMARY KEY, " // Concatenation of scope + tag
> +    "origin TEXT NOT NULL, "
> +    "originSuffix TEXT NOT NULL, "

Why is this a separate column?  I believe nsIPrincipal::GetOrigin() returns a single string that includes the suffix.

@@ +32,5 @@
> +    "id TEXT NOT NULL PRIMARY KEY, " // Concatenation of scope + tag
> +    "origin TEXT NOT NULL, "
> +    "originSuffix TEXT NOT NULL, "
> +    "scope TEXT NOT NULL, "
> +    "tag TEXT NOT NULL, "

This will have encoding errors if script passes certain values for the tag DOMString.  You can use BLOB instead of TEXT to avoid sqlite doing encoding conversions here.  Note that you have to allow NULL for "" values, though.  Also see how dom/cache reads and writes these columns.

@@ +202,5 @@
> +  MOZ_ASSERT(aConn);
> +
> +  nsCOMPtr<mozIStorageStatement> state;
> +  nsresult rv = aConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT * FROM registrations;"

Since we have to match indices to columns below I think its a bit better to explicitly list the columns here instead of *.  That helps avoid accidental mis-matching of columns if the schema definition changes.

@@ +269,5 @@
> +  int32_t count = 0;
> +  bool hasMoreData = false;
> +  while (NS_SUCCEEDED(getOriginState->ExecuteStep(&hasMoreData))
> +         && hasMoreData) {
> +    // We only set the origin if there's one registration.

This code is setting the origin on the first registration, not if there is only one total.  Is that what this comment should say?

::: dom/backgroundsync/DBSchemaUtils.h
@@ +19,5 @@
> +nsresult
> +CreateOrMigrateSchema(mozIStorageConnection* aConn,
> +                      int32_t aLatestSchemaVersion,
> +                      const char* aTableSql,
> +                      const char* aExpectedTableName);

I feel like this code is a good candidate to move to mozstorage or the like.  Its practically identical to the dom/cache version.

::: dom/backgroundsync/StorageManager.cpp
@@ +66,5 @@
> +
> +    RefPtr<StorageManager> ref = Get(aManagerId);
> +    if (!ref) {
> +      nsCOMPtr<nsIThread> ioThread;
> +      rv = NS_NewNamedThread("BSyncIOThread", getter_AddRefs(ioThread));

I guess this is ok, but I hope to change dom/cache not to do this.  I think you can use a TaskQueue pointing at a thread pool now.  That will limit the number of threads used while providing deterministic ordering of events.

@@ +328,5 @@
> +                  mozIStorageConnection* aConn) override
> +  {
> +    nsresult rv = db::Register(aConn, mArgs, mFirstRegistration,
> +                               mRegistration);
> +    NS_WARN_IF(NS_FAILED(rv));

I think this recently changed to NS_WARNING_ASSERTION(NS_FAILED(rv)) for these kinds of calls outside of if-statements.

::: dom/backgroundsync/StorageManagerId.cpp
@@ +99,5 @@
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(aListener);
> +  MOZ_ASSERT(!mListenerList.Contains(aListener));
> +
> +  mListenerList.AppendElement(aListener);

This is racy.  What if AddListener() is called after you've already gotten the ID from the main thread?  The listener will never be called.  Seems like we should at least assert we haven't notified the listeners about the ID yet.

::: dom/backgroundsync/StorageManagerId.h
@@ +46,5 @@
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(StorageManagerId)
> +};
> +
> +class StorageManagerIdFactory : public Runnable

Its a bit weird from a public interface point of view to make this a Runnable directly.  Recommend using an anonymous class or NS_NewRunnableMethod in the .cpp file instead.

@@ +61,5 @@
> +    virtual void OnStorageManagerIdCreated(StorageManagerId* aManagerId) = 0;
> +  };
> +
> +  static already_AddRefed<StorageManagerIdFactory>
> +  Create(Listener* aListener, const PrincipalInfo& aPrincipalInfo);

I realize you copied this from dom/cache, but perhaps a better way to do it now would be with a MozPromise.  I believe you can have multiple consumers call .Then() and get the value regardless of timing, etc.  This is fine for now, though.

@@ +63,5 @@
> +
> +  static already_AddRefed<StorageManagerIdFactory>
> +  Create(Listener* aListener, const PrincipalInfo& aPrincipalInfo);
> +
> +  void AddListener(Listener* aListener);

Can we remove the Create() listener arg or the AddListener() method?  It would be nice if there was only one way to add a listener.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +514,5 @@
>  // -----------------------------------------------------------------------------
>  
> +dom::backgroundsync::PBackgroundSyncChild*
> +BackgroundChildImpl::AllocPBackgroundSyncChild(
> +    const PrincipalInfo& aPrincipalInfo)

If you are not consuming the principal in your actor impl class then you can just leave off the name here and fit this all on one line.

::: ipc/glue/IPCMessageUtils.h
@@ +829,5 @@
> +using namespace mozilla::dom::backgroundsync;
> +
> +template <>
> +struct ParamTraits<RegistrationState>
> +  : public ContiguousEnumSerializer<RegistrationState,

I think this can just go in your BackgroundSyncTypes.h header file.
Comment 99 User image Jan Varga [:janv] 2016-09-12 09:34:22 PDT
Comment on attachment 8788567 [details] [diff] [review]
Part 5: QuotaClient helpers

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

I just quickly went through this patch, but I don't think this is generic enough to live in dom/quota.
For example it uses mozIStorageConnection, but a quota client is free to use any database system, even plain text files for its storage.
However if you give me strong arguments for dom/quota placement I might change my mind.
Again, I didn't read all the comments in this bug.
Comment 100 User image Ben Kelly [:bkelly] 2016-09-12 09:56:29 PDT
(In reply to Jan Varga [:janv] from comment #99)
> I just quickly went through this patch, but I don't think this is generic
> enough to live in dom/quota.
> For example it uses mozIStorageConnection, but a quota client is free to use
> any database system, even plain text files for its storage.
> However if you give me strong arguments for dom/quota placement I might
> change my mind.
> Again, I didn't read all the comments in this bug.

It has one place that allows us to share an existing storage connection, but otherwise doesn't mention connections at all.  And it works with files.

These are basically the Context and Action bits from dom/cache pulled out so bg-sync can use them.  They solely exist to deal with the complexities required by QuotaManager.
Comment 101 User image Jan Varga [:janv] 2016-09-27 06:23:07 PDT
I'm going through the patches to better understand this implementation ...
Comment 102 User image Jan Varga [:janv] 2016-09-27 23:53:36 PDT
Comment on attachment 8788560 [details] [diff] [review]
Part 3: IPC

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

::: dom/backgroundsync/PBackgroundSync.ipdl
@@ +15,5 @@
> +{
> +  manager PBackground;
> +
> +parent: // child -> parent messages
> +  async Request(nsID aRequestId, SyncOp aOp);

Have you considered to create a sub protocol for requests instead of this Request()/Response() pair ?
Comment 103 User image Jan Varga [:janv] 2016-09-28 01:02:50 PDT
Ok, so given comments by asuth and bkelly about factoring out generic stuff. Some of the code should go to mozStorage and a common dom/ directory.
asuth suggested dom/common, dom/shared, dom/quota/clients or something else.
I mostly agree with that except that I still don't think it should go to dom/quota or dom/quota/clients.
The quota helpers are mostly tailored for dom/cache and dom/backgroundsync. I can't imagine that other quota clients would use these helpers. IndexedDB quota initialization has its own specifics and is even more complex than dom/cache, on the other hand dom/asmjscache is much simpler. Local Storage will have its own specifics (sync API).
Once we have time to do some cleanup work, Quota Manager public API should be simplified a bit to make it easier for clients to initialize quota stuff.
Comment 104 User image Jan Varga [:janv] 2016-09-28 01:04:22 PDT
Comment on attachment 8788569 [details] [diff] [review]
Part 6: Storage

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

::: dom/quota/Client.h
@@ +17,5 @@
>  
>  #define IDB_DIRECTORY_NAME "idb"
>  #define ASMJSCACHE_DIRECTORY_NAME "asmjs"
>  #define DOMCACHE_DIRECTORY_NAME "cache"
> +#define BACKGROUNDSYNC_DIRECTORY_NAME "backgroundsync"

I would probably shorten this to "bgsync"

@@ +42,5 @@
>      //LS,
>      //APPCACHE,
>      ASMJS,
>      DOMCACHE,
> +    BACKGROUNDSYNC,

here too
Comment 105 User image Jan Varga [:janv] 2016-09-28 01:06:40 PDT
It also seems to me that you need to share some storage related stuff between dom/cache and dom/backgroundsync, so you will have to create a shared folder anyway.

Not sure about a good name for it, maybe dom/storage-common ?
Comment 106 User image Ben Kelly [:bkelly] 2016-09-28 08:08:43 PDT
I still think this is overwhelmingly quota specific, but ok.  A lot of shared stuff goes in dom/base, but it might be nice to not pile on there.  We could instead put these shared files in "dom/shared" or something. Or "dom/util".

Olli, do you have an opinion on where to put code shared between DOM modules?
Comment 107 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2016-09-28 08:27:50 PDT
I'm not familiar with this code at all, but if it is mostly quota specific, I would put the code close to the quote code. Adding yet another directory just for storage specific utilities... doesn't sounds too good.
Comment 108 User image Jan Varga [:janv] 2016-09-28 09:06:28 PDT
I'm ok with something like dom/cache/something or if that's not acceptable dom/quota/something.
I'll think about suitable name.
Comment 109 User image Ben Kelly [:bkelly] 2016-09-28 09:07:46 PDT
dom/quota/shared or dom/quota/util would be my vote.
Comment 110 User image Jan Varga [:janv] 2016-09-28 11:53:38 PDT
dom/quota/shared sounds good
Comment 111 User image Andrea Marchesini [:baku] 2016-11-04 05:38:42 PDT
Comment on attachment 8788574 [details] [diff] [review]
Part 8: BackgroundSyncService

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

Sorry, sorry, for the delay!

::: dom/backgroundsync/BackgroundSyncService.cpp
@@ +33,5 @@
> +
> +// ----------------------------------------------------------------------------
> +
> +class GetStorageManagerRunnable final : public nsIRunnable
> +                                      , public nsICancelableRunnable

Just use nsCancelableRunnable and remove these 2 lines, NS_DECL_THREADSAFE_ISUPPORTS, and NS_IMPL_ISUPPORTS(GetStorageManagerRunnable, nsICancelableRunnable, nsIRunnable) and Cancel() { ...

@@ +42,5 @@
> +  GetStorageManagerRunnable(BackgroundSyncService* aService,
> +                            const nsString& aOrigin,
> +                            const SyncInternalOp& aOp)
> +    : mService(aService)
> +    , mOrigins(nsTArray<nsString>())

this is not needed.

@@ +75,5 @@
> +
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsIScriptSecurityManager *securityManager =
> +      nsContentUtils::GetSecurityManager();

are you sure this is > 80 chars?

@@ +85,5 @@
> +    // StorageManagers for a GetAll operation.
> +    for (uint32_t i = 0; i < mOrigins.Length(); i++) {
> +      nsCOMPtr<nsIPrincipal> principal;
> +      securityManager->CreateCodebasePrincipalFromOrigin(
> +        NS_ConvertUTF16toUTF8(mOrigins[i]), getter_AddRefs(principal));

this can fail.

@@ +425,5 @@
> +        AddChromeStorageManagerRef(aRequestId, manager);
> +        manager->Register(aRequestId,
> +            aResponse.get_SyncRegisterResponse().mRegistration());
> +        return;
> +      } else {

no } else { after |return|

@@ +467,5 @@
> +        // StorageManager for each origin by jumping to the main thread.
> +        // Once we have each appropriate StorageManager, the GetAll operation
> +        // will be executed at OnStorageManagerIdCreated. That's why we pass here
> +        // the SyncInteralOp with SyncGetAllArgs.
> +        nsTArray<nsString> origins(

can this be:
nsTArray<nsString>& origins = aResponse.get_SyncGetAllOriginsResponse().mOrigins(); ?

same for the other arrays.

@@ +560,5 @@
> +  RefPtr<GetStorageManagerRunnable> runnable =
> +    new GetStorageManagerRunnable(this, aRegistration.mOrigin(), SyncInternalOp(
> +      SyncChangeStateArgs(aRegistration.mId(),RegistrationState::FIRING)
> +    ));
> +  MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable));

this can fail actually :/
Here and all the others NS_DispatchToMainThread()

::: dom/backgroundsync/BackgroundSyncService.h
@@ +28,5 @@
> +class StorageManager;
> +class SyncOp;
> +
> +class BackgroundSyncService final : public OnlineStateObserver::Listener
> +                                  , StorageManagerIdFactory::Listener

why not public?

::: dom/backgroundsync/ChromeStorageManager.cpp
@@ +304,5 @@
>  }
>  
>  // static
>  already_AddRefed<ChromeStorageManager>
> +ChromeStorageManager::GetOrCreate(BackgroundSyncService* aService)

What about GetOrCreate() without params? the backgroundSyncService is a singleton, right?

@@ +316,1 @@
>    }

else {
  MOZ_ASSERT(csmInstance->GetBackgroundSyncService() == aService);
  MOZ_ASSERT(aService == BackgroundSyncService::GetOrCreate());
}

We don't want multiple services...

::: dom/backgroundsync/StorageManagerId.cpp
@@ +41,5 @@
>  
>    return factory.forget();
>  }
>  
> +

why an extra line here?

::: dom/backgroundsync/moz.build
@@ +2,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  EXPORTS.mozilla.dom.backgroundsync += [

I don't think we want an additional namespace here. Any reason why we want this?

::: dom/workers/ServiceWorkerManagerParent.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "BackgroundSyncService.h"

mozilla/dom/backgroundsync/BackgroundSyncService.h

@@ +330,5 @@
>  
>    mActorDestroyed = true;
>  
> +  if (mServiceWorkerService) {
> +    // This object is about to be released and with it, also mServiceWorkerService will be

80chars?

::: dom/workers/moz.build
@@ +94,5 @@
>  
>  LOCAL_INCLUDES += [
>      '../base',
>      '../system',
> +    '/dom/backgroundsync',

you don't need this.

Note You need to log in before you can comment on or make changes to this bug.