1217544 - Web BackgroundSync API SyncManager implementation

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Andrew Overholt [:overholt]]

https://github.com/slightlyoff/BackgroundSync/

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I hope you meant after v1.

Comment 2

[Andrew Overholt [:overholt]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> I hope you meant after v1.

Oops, yeah.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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

[Fernando Jiménez Moreno [:ferjm]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL (WIP)

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration (WIP)

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC (WIP)

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Andrew Sutherland [:asuth] (he/him)]

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

[Ben Kelly [:bkelly, not reviewing]]

I think it will depend on the underlying platform.  Android facilities may differ from whats available on b2g.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC (WIP)

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC (WIP)

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff (WIP)

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration (WIP)

Now it works on workers.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC (WIP)

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff (WIP)

Comment 21

[Andrea Marchesini [:baku]]

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

[Fernando Jiménez Moreno [:ferjm]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL

Updated with Andrea's feedback addressed. I also took the liberty of alphabetically ordering the entire moz.build.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration

Updated with Andrea's feedback.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC

Updated with Andrea's feedback.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: SyncService (WIP)

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: SyncEvent

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: SyncEvent

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: High level architecture

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

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

[Andrea Marchesini [:baku]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: Sync DB

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: SyncService

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff (WIP)

And now with the correct patch. Sorry for the noise.

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL and SyncManager

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: SyncEvent

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: Sync DB

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: SyncService

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff

Comment 47

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Fernando Jiménez Moreno [:ferjm]]

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

[Andrea Marchesini [:baku]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff

Unified diff with the WIP refactor

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL and DOM bindings

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: OnlineObserver

Comment 61

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: BackgroundSyncService

Comment 62

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 8: Tests

Comment 63

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff

Comment 64

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

I gave you r+, but I want to see a interdiff with WorkerFeature to WorkerHolder.

Comment 66

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrea Marchesini [:baku]]

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

[Andrew Sutherland [:asuth] (he/him)]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Andrew Sutherland [:asuth] (he/him)]

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

[Fernando Jiménez Moreno [:ferjm]]

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

[Ben Kelly [:bkelly, not reviewing]]

(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

[Fernando Jiménez Moreno [:ferjm]]

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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL and DOM bindings

Rebased on top of latest m-i. Keeping baku's r+ after comment 63

Comment 77

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration

Rebased as well. r+ from comment 48.

Comment 78

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC

Addressed Andrea's feedback from comment 64. Interdiff with WorkerFeature to WorkerHolder on the way.

Comment 79

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3.1. : Interdiff with WorkerFeature to WorkerHolder

Interdiff with WorkerFeature to WorkerHolder as requested in comment 65

Comment 80

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Sync event

Rebased. r+ from comment 50

Comment 81

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: OnlineObserver

Addressed Andrea's feedback. r+ from comment 66.

Comment 82

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8777388 [details] [diff] [review]
Part 4: Sync event

r+ from comment 50 (now for real)

Comment 83

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: BackgroundSyncService

Feedback from comment 67 addressed

Comment 84

[Fernando Jiménez Moreno [:ferjm]]

(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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL and DOM bindings

Rebased on top of m-i.

r+ from comment 63.

Comment 86

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: ServiceWorkerRegistration

r+ from comment 48.

Comment 87

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: IPC

r+ from comment 64

Comment 88

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Sync event

r+ from comment 50.

Comment 89

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: 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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 8: OnlineObserver

r+ from comment 66.

Comment 92

[Ben Kelly [:bkelly, not reviewing]]

(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

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 9: BackgroundSyncService

Rebased and addresses feedback from comment 67

Comment 94

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

r+ from comment 68.

Comment 95

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff

Comment 96

[Fernando Jiménez Moreno [:ferjm]]

(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

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Jan Varga [:janv]]

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

[Ben Kelly [:bkelly, not reviewing]]

(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

[Jan Varga [:janv]]

I'm going through the patches to better understand this implementation ...

Comment 102

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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

[Ben Kelly [:bkelly, not reviewing]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Jan Varga [:janv]]

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

[Ben Kelly [:bkelly, not reviewing]]

dom/quota/shared or dom/quota/util would be my vote.

Comment 110

[Jan Varga [:janv]]

dom/quota/shared sounds good

Comment 111

[Andrea Marchesini [:baku]]

Comment on attachment 8788574 [details] [diff] [review]
Part 9: 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.

Comment 112

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1.1 (interdiff): WebIDL and DOM bindings

Comment 113

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3.1 (interdiff): IPC

Comment 114

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4.1 (interdiff): Sync event

Comment 115

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 8.1 (interdiff): OnlineStateObserver

Comment 116

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 9.1 (interdiff): BackgroundSyncService

Comment 117

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: QuotaClient helpers

Comment 118

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: mozStorageConnectionUtils

Comment 119

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Storage

Comment 120

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Unified diff

Comment 121

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests (interdiff)

Comment 122

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8832156 [details] [diff] [review]
Part 5: QuotaClient helpers

This patch is still as described in comment 89 but addressing the feedback from comment 97 and following. I moved the code to dom/quota/shared as suggested.

Comment 123

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8832157 [details] [diff] [review]
Part 6: mozStorageConnectionUtils

This patch extracts the storage related parts shared by dom/cache and dom/backgroundsync to mozStorage as suggested in comment 98.

The patch to use this code in dom/cache is attached to bug 1308447.

Comment 124

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 125

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832156 [details] [diff] [review]
Part 5: QuotaClient helpers

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

I'm sorry, some changes have occurred since you initially did this.  Please make sure to incorporate the fixes from here:

https://hg.mozilla.org/mozilla-central/rev/46cb4ee49786

And here:

https://hg.mozilla.org/mozilla-central/rev/a522c83358fa

Comment 126

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832157 [details] [diff] [review]
Part 6: mozStorageConnectionUtils

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

Redirecting this to :asuth as a storage peer.

Comment 127

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832158 [details] [diff] [review]
Part 7: Storage

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

Can you reflag this review with a description of the patch's purpose?  Its not clear to me what its doing here and why.  Thanks!

Comment 128

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #125)
> I'm sorry, some changes have occurred since you initially did this.  Please
> make sure to incorporate the fixes from here:

As a de-bitrotting tip for refactors like this[1], I've found that using a clever diff/merge tool like meld (http://meldmerge.org/) can be a great way to cleanup similar bit-rot.  Specifically I do "meld TRUNK/oldpath/oldfile MYBRANCH/newpath/newfile" and I find meld is usually able to synchronize sufficiently and then the intra-line diffing and clicky arrows make it easy for me to either just propagate the change with a single click, or confidently apply the relevant manual transformation without accidentally carrying over other changes.

1: You may already have a strategy for this.  If yours isn't the same, I'd definitely be interested in hearing it!

Comment 129

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8832157 [details] [diff] [review]
Part 6: mozStorageConnectionUtils

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

Thank you very much for extracting these pieces out!  Since this is largely an extraction of existing, working code with refactoring to support parameterization, I don't really have any feedback to offer.  (Note: I'm not suggesting this was a small undertaking; major props for your efforts here!)  I'm requesting feedback? from :mak so he's aware of this existing on the horizon, but I expect it makes the most sense for any mozStorage concerns to be addressed in follow-up bugs in the Storage component after the patches on this bug and bug 1308447 have landed so we don't complicate an already large patch-series.  (I think renaming/etc. will be much easier once things are already in the tree.)

I think the main judgement calls here are naming, and I agree with them, namely:
* File/class names: mozStorageconnectionConnectionUtils appropriately parallels mozStorageConnection, and IncrementalVacuumConnection matches the policy that we don't bother prefixing new stuff with mozStorage.
* mozilla::storage::utils seems like a fine place to stash opt-in functionality that tries to have simple names like Expect, Migrate, etc.  If we end up adopting the migration functionality more broadly (places? :), we can revisit naming.

Change tracking:

Straightforward extracted/renamed pieces with only minor naming changes:
* dom/cache/Connection => IncrementalVacuumConnection
* Pieces from dom/cache/DBSchema => mozStorageConnectionUtils:
  * struct Expect
  * AutoDisableForeignKeyChecking
  * IncrementalVacuum

Refactorings from dom/cache/DBSchema => mozStorageConnectionUtils:
* Validate: hardcoded expect list made into aExpectedSchema
* RewriteSchema: hardcoded table name made into aTable
* Migrate: version constants and migration list made parameters
* CreateOrMigrateSchema: version constants, table/index creation SQL made parameters.  Validate and Migrate arguments also propagated through.
* InitializeConnection: page size, growth size, wal checkpoint constants made parameters.

::: storage/mozStorageConnectionUtils.cpp
@@ +125,5 @@
> +  return rv;
> +}
> +
> +nsresult
> +RewriteSchema(mozIStorageConnection* aConn,

I suggest we add the contextual comment:

// As documented at https://www.sqlite.org/lang_altertable.html#otheralter
// for use when: "removing CHECK or FOREIGN KEY or NOT NULL constraints,
// renaming columns, or adding or removing or changing default values on a
// column."

::: storage/mozStorageConnectionUtils.h
@@ +38,5 @@
> +  const nsCString mSql;
> +  const bool mIgnoreSql;
> +};
> +
> +typedef nsresult (*MigrationFunc)(mozIStorageConnection*, nsACString&,

It would be great to add a brief comment along these lines:

// Args are: mozIStorageConnection* aConn, nsACString& aSchemaRewriteTable,
// nsACString& aSchemaRewriteSql.
// You can find detailed use of this support in dom/cache/DBSchema.cpp

Comment 130

[Marco Bonardo [:mak]]

Comment on attachment 8832157 [details] [diff] [review]
Part 6: mozStorageConnectionUtils

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

Thank you for the heads-up!
I have nothing against moving existing code to Storage to better share it, but I have some questions regarding the contents of this. Clearly if this is already existing code it's fine to move it as-is and act later, as well as some of my questions could just be answered negatively.

1. Why the need for another connection type, wouldn't an attribute on mozIStorageconnection achieve the same result? It's possible you have future plans that I'm not aware of, but as things are I don't see why an existing connection couldn't grow an attribute to indicate it as "special".
2. Based on the implementation, I assume this is intended to always work off the main-thread, thus I'd appreciate some MOZ_ASSERT(!NS_IsMainThread()) in its methods.
3. It would be great if header files had some inline documentation about what things are and what they can be used for. Especially for main classes and helpers.
4. Just to be sure everyone is aware, we should maybe point out in a comment (maybe the inline documentation of IncrementalVacuum?) that auto_vacuum doesn't resolve internal db fragmentation, and for that a full vacuum is still needed. I'd not want people thinking this is THE solution to fragmentation. It's a good solution to filesystem fragmentation, doesn't help at all with db fragmentation.
5. The migration system seems to a bit lacking regarding downgrades, basically if the current version is bigger than the expected one. Afaict it seems to handle it the same as if the versions are identical. It may not be something you need now, I was mostly guessing it would be useful to be able to choose an action for such case in the future.
6. SetSchemaVersion seems to only be invoked in the "no schema" case, shouldn't it be set even after a migration to a newer version?

note, I quickly read through this, since Andrew already reviewed it, so I may have missed something that answers my questions already.

Comment 131

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Marco Bonardo [::mak] from comment #130)
> 1. Why the need for another connection type, wouldn't an attribute on
> mozIStorageconnection achieve the same result? It's possible you have future
> plans that I'm not aware of, but as things are I don't see why an existing
> connection couldn't grow an attribute to indicate it as "special".

I agree this is a good follow-up change.  Especially since it would be great for the functionality to integrate with my proposed "don't do I/O when closing at shutdown" optimization at https://bugzilla.mozilla.org/show_bug.cgi?id=1186276#c22.  The one complication to such an optimization is that this is per-origin storage, so quota manager would ideally need functionality to help its clients perform opportunistic vacuuming/non-visible-to-content eviction of derived data.  I'd expect that to become relevant when bug 1336199 gets addressed since the alternate data streams can be purged at-will.

> 5. The migration system seems to a bit lacking regarding downgrades,
> basically if the current version is bigger than the expected one. Afaict it
> seems to handle it the same as if the versions are identical. It may not be
> something you need now, I was mostly guessing it would be useful to be able
> to choose an action for such case in the future.

Content-facing APIs with persistent storage/semantics are largely unable to support downgrades, see bug 1246615.  It's probably best for consumers that can safely downgrade (and want to make the effort in the face of the content APIs that don't) to drive this effort.

> 6. SetSchemaVersion seems to only be invoked in the "no schema" case,
> shouldn't it be set even after a migration to a newer version?

Currently all the migration functions call SetSchemaVersion themselves.  See the DBSchema.cpp changes in the patch on bug 1308447.  Additionally, the upgrade loop asserts if the schema version didn't increase in some fashion in a pass through the loop.  I think this potentially allows upgrade functions more latitude, but is an indicator of where documentation with comments would be appropriate.

Comment 132

[Marco Bonardo [:mak]]

(In reply to Andrew Sutherland [:asuth] from comment #131)
> > 6. SetSchemaVersion seems to only be invoked in the "no schema" case,
> > shouldn't it be set even after a migration to a newer version?
> 
> Currently all the migration functions call SetSchemaVersion themselves.  See
> the DBSchema.cpp changes in the patch on bug 1308447. 

Is there a specific reason to not do this in the helper, since in the end it already knows which version we are migrating to? Sounds like boilerplate to me.

Comment 133

[Andrea Marchesini [:baku]]

Comment on attachment 8832153 [details] [diff] [review]
Part 4.1 (interdiff): Sync event

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1107,5 @@
>                                 aBody, aTag, aIcon, aData, aBehavior);
>  }
>  
>  NS_IMETHODIMP
> +ServiceWorkerManager::SendBackgroundSyncEvent(const nsACString& aOrigin,

To avoid confusion about what we mean with 'origin', would be nice to pass the principal here.
This can be a follow up, or, just write a good comment here.

Comment 134

[Andrea Marchesini [:baku]]

Comment on attachment 8832155 [details] [diff] [review]
Part 9.1 (interdiff): BackgroundSyncService

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

::: dom/backgroundsync/BackgroundSyncService.cpp
@@ +254,5 @@
>  BackgroundSyncService::MaybeShutdown()
>  {
>    AssertIsOnBackgroundThread();
>  
>    if (mBackgroundSyncActors.Length() == 0 &&

IsEmpty()

@@ +255,5 @@
>  {
>    AssertIsOnBackgroundThread();
>  
>    if (mBackgroundSyncActors.Length() == 0 &&
> +      mServiceWorkerManagerActors.Length() == 0 &&

IsEmpty()

@@ +474,5 @@
>          RefPtr<GetStorageManagerRunnable> runnable =
>            new GetStorageManagerRunnable(this, origins,
>                SyncInternalOp(SyncGetAllArgs()));
> +        nsresult rv = NS_DispatchToMainThread(runnable);
> +        if (NS_FAILED(rv)) { NS_WARNING("NS_DispatchToMainThread failed"); }

... {
  NS_WARNING("Failed to dispatch a GetStorageManagerRunnable to the main-thread.");
}

@@ +515,5 @@
>              new GetStorageManagerRunnable(this, registration.mOrigin(),
>                  SyncInternalOp(SyncChangeStateArgs(registration.mId(),
> +                               RegistrationState::ePending)));
> +          nsresult rv = NS_DispatchToMainThread(runnable);
> +          if (NS_FAILED(rv)) { NS_WARNING("NS_DispatchToMainThread failed"); }

same here.

@@ +566,3 @@
>      ));
> +  nsresult rv = NS_DispatchToMainThread(runnable);
> +  if (NS_FAILED(rv)) { NS_WARNING("NS_DispatchToMainThread failed"); }

here.

@@ +582,3 @@
>          ));
> +      nsresult rv = NS_DispatchToMainThread(runnable);
> +      if (NS_FAILED(rv)) { NS_WARNING("NS_DispatchToMainThread failed"); }

here

@@ +601,5 @@
>    RefPtr<GetStorageManagerRunnable> runnable =
>      new GetStorageManagerRunnable(this, aRegistration.mOrigin(),
>          SyncInternalOp(SyncRemoveArgs(aRegistration.mId())));
> +  nsresult rv = NS_DispatchToMainThread(runnable);
> +  if (NS_FAILED(rv)) { NS_WARNING("NS_DispatchToMainThread failed"); }

... all of them.

Comment 135

[Andrew Sutherland [:asuth] (he/him)]

We're having a Service Worker work week this week in Taipei and are hoping to take the patches the last few feet to the finish line and land it this week.  So I'm stealing the bug to cover that.  :bkelly, :catalin, :hopang, and :tt may also be involved.

I'm assuming these patches represent the current state of the patch.  :ferjm if you see this and have local changes that you can easily make available please let us know!  Thanks!

Comment 136

[Fernando Jiménez Moreno [:ferjm]]

Thanks for following up with this work Andrew. Yes, what's attached to the bug contains the current state of the patch.

Comment 137

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Andrew, can you tell us whether this will need any Manual testing? If yes, could you please provide some details as to what you think should be tested?

I see there are automated tests already covering this implementation.

Comment 138

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 139

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Multi-origin tests

Comment 140

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Part 10: IPC, multi-origin, and crash resiliency fixes

IPC:
- Don't send the sync event to all child content processes.  Instead,
  pick a single ServiceWorkerManager to tell to dispatch the event.
  For registrations that immediately fire an event, use the SWM
  corresponding to the process that the request originated from.  For
  online transition cases, pick the first content process, creating one
  if e10s mode is enabled and one doesn't exist.
- Ensure permissions are available before potentially causing the
  ServiceWorkerPrivate to be spun up using the bug 1355608 mechanism.

multi-origin:
- GetStorageManagerRunnable did not handle the multi-origin case
  correctly.  It mutated its single mPrincipalInfo in a tight loop during
  which it dispatched itself to the main thread to consume mPrincipalInfo.
  Depending on scheduling, seeing all the requests as involving the same
  origin would be possible
- GetStorageManagerRunnable was also a bit silly.  It would go to the main
  thread to convert an origin to a Principal and then into a PrincipalInfo.
  When it got back to the background thread, it would use
  StorageManagerIdFactory::Create which would then go to the main thread
  to convert the PrincipalInfo back into a Principal and actually create
  the StorageManagerId.
- A simple fix was made possible by eliminating the wasteful extra bounce
  to the background and then back to the main thread by introducing a
  variant of StorageManagerIdFactory::CreateOnMainThread for use by the
  GetStorageManagerRunnable which then no longer needs to worry about
  dealing with the multiplicity.

crash resiliency:
- Before dispatching the "sync" event, the BackgroundSyncService would
  change the state of the registration to "firing" on disk.  A "firing"
  registration would not fire again.  If the system went offline again
  before the state change returned, the state would be changed back to
  "pending".  But no logic addressed the case where a crash occurred
  and at next startup the system came up with registrations already in
  the firing state.  The only way to recover from this state would be
  for user code to remove the existing sync and re-register.  Removing
  would succeed, but duplicative register() calls would fail because
  of primary key collisions.
- The "fix" for this, which also happened to simplify propagating the
  actor for the IPC process-affinity for firing the event, is to just
  maintain an in-memory map of all currently pending firing events.
  This is currently somewhat overkill since the simplification also
  allowed the entire dispatch to become synchronous with the removal
  happening later in the method.  However, when the follow-on
  bug 1260141 fix happens, the map is arguably the right way to handle
  that still, although it wants a little bit of fleshing out to deal
  with the reregisteredWhileFiring state.  As far as the disk state
  is concerned, all that matters is whether a sync should happen at
  next startup and any back-off or last chance factors.  The largest
  complication is that registration id generation currently only
  happens in the database layer which does complicate single source of
  truth notions.  However, that can be left as-is if the "registration
  already exists" case is propagated back to the PBackground thread
  with the actual current registration and its id, allowing the map
  to be properly fixed up or the request re-issued if the map no longer
  has an entry.  Alternately, the PBackground state can be made a
  more authoritative source of truth for situations like that by having
  it track pending requests issued to the managers until they
  complete.  While there's something to be said for the managers and IO
  thread being a single source of truth, it's massive overkill for
  this use-case where really a simple async key-value store is all that's
  needed.

Comment 141

[Andrew Overholt [:overholt]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #137)
> Andrew, can you tell us whether this will need any Manual testing? If yes,
> could you please provide some details as to what you think should be tested?

ATM we don't have anything because we don't know of sites using this that could be tested. If anyone playing along at home reads this and wants to chime in with a URL, we'd appreciate it!

I guess the answer is 'no' until the above changes. Thanks, Florin!

Comment 142

[Marco Castelluccio [:marco]]

A couple of demo links which might work:
https://deanhume.github.io/Service-Workers-BackgroundSync/
https://github.com/WICG/BackgroundSync/tree/master/demo
https://developers.google.com/web/updates/2015/12/background-sync (there are two demos linked in the article)

Comment 143

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thank you guys for the answers! 

One more question - should this also work on Android?

Comment 144

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 145

[Andrew Sutherland [:asuth] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dedfd78e0da3bd56cbf8d90cd5cf27240f7f19b

Comment 146

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 147

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 148

[Florian Bender]

Hey Andrew, what's the status and forward plan here? I'd love to have this in the next ESR v60. Is that viable?

Comment 149

[Andrew Sutherland [:asuth] (he/him)]

Cleaning up the multi-e10s complications for backgroundsync was getting complex so this patchset started waiting on finishing up our multi-e10s refactor for ServiceWorkers.  We're hoping to land those changes for v60, but we plan on keeping ServiceWorkers disabled on the ESR.  backgroundsync will likely end up in 61.  But it also may end up waiting on ServiceWorkerRegistrar/QuotaManager storage enhancements so we can avoid all the downgrade-cliff fallout that adding a new QuotaManager client and separate database currently entails in a world without profile-per-channel.

Comment 150

[Tim Langhorst]

(In reply to Andrew Sutherland [:asuth] from comment #149)
> Cleaning up the multi-e10s complications for backgroundsync was getting
> complex so this patchset started waiting on finishing up our multi-e10s
> refactor for ServiceWorkers.  We're hoping to land those changes for v60,
> but we plan on keeping ServiceWorkers disabled on the ESR.  backgroundsync
> will likely end up in 61.  But it also may end up waiting on
> ServiceWorkerRegistrar/QuotaManager storage enhancements so we can avoid all
> the downgrade-cliff fallout that adding a new QuotaManager client and
> separate database currently entails in a world without profile-per-channel.

So we are 63, a anything happened since 60?

Comment 151

[Andrew Sutherland [:asuth] (he/him)]

It's been a long road, but the ServiceWorker e10s overhaul should ideally be landing in 63.  For BackgroundSync, we believe it to be a good use-case for providing a simpler mechanism for APIs that operate in term of ServiceWorker registrations to hang their data off of.  Particularly as it relates to tracking the set of origins with pending sync events.

So we may gate landing this on that.  There's an element of "the perfect is the enemy of the good" to that approach, but the reality is that we've been having issues with brittle-ness of QuotaManager/IndexedDB as it relates to upgrades breaking things, and it seems most practical to avoid landing a temporary storage scheme that would need non-trivial upgrade logic to port to something safer later.

Comment 152

[Marion Daly [:mdaly]]

(In reply to Andrew Sutherland [:asuth] from comment #151)
> It's been a long road, but the ServiceWorker e10s overhaul should ideally be
> landing in 63.  For BackgroundSync, we believe it to be a good use-case for
> providing a simpler mechanism for APIs that operate in term of ServiceWorker
> registrations to hang their data off of.  Particularly as it relates to
> tracking the set of origins with pending sync events.
> 
> So we may gate landing this on that.  There's an element of "the perfect is
> the enemy of the good" to that approach, but the reality is that we've been
> having issues with brittle-ness of QuotaManager/IndexedDB as it relates to
> upgrades breaking things, and it seems most practical to avoid landing a
> temporary storage scheme that would need non-trivial upgrade logic to port
> to something safer later.

Perhaps we should reevaluate this as we did not land in 63.

Comment 153

[Marion Daly [:mdaly]]

Asuth, your thoughts?

Comment 154

[Andrew Sutherland [:asuth] (he/him)]

(I also expressed this in a meeting, but for posterity:)
There were 2 reasons to delay:
1. Process/SW selection will be easier after sw-e10s lands because there is no longer any process selection.  This is what bug 1368625 was about that I've now WONTFIXED.
2. There is a large amount of complexity around database storage in the implementation out of necessity due to a lack of alternatives.  Given the expected future needs of other APIs it made sense to try and wait, especially given the massive issues we'd been having around upgrades/downgrades that are still currently an issue given how our profile management works.  (Bug 1474285 should now be the long term fix for the "profile from the future").  In terms of concrete progress being made, bug 1474285 is introducing a kvstore that we can use for the per-origin needs and potentially have QuotaManager provide some explicit convenience logic to provide access to kvstores on a per-origin basis.  Ideally QM could also provide the "these origins have data for your client" info without backgroundsync needing to maintain its own data-store, but we haven't made progress towards that yet.

Issue 1 still makes sense to defer landing for, and issue 2 is still also a very good reason not to land.

I'm marking this DWS_NEXT for visibility, but we won't be able to get to this for a few months I think.

Comment 155

[Eden Chuang[:edenchuang]]

Attachment: Bug 1217544 - P1 SyncManager webidl, DOM binding and IPC implementation

WebBackgroundSync API implementation. Part 1.

1. SyncManager webidl and DOM binding code implementation.
2. IPC implementation for serviceWorkerRegistration.sync.register and
   serviceWorkerRegistration.sync.getTags

Comment 156

[Eden Chuang[:edenchuang]]

Separating the WebBackgroundSync API implementation into followings

1. SyncManager API webidl binding and IPC implementation.
2. SyncEvent API webidl binding and IPC implementation.
3. Storage support for saving SyncRegistration.
4. WebBackgroundSync OnlineObserver implementation.
5. User permission for BackgroundSync API

And this bug is for tracking 1.

Comment 157

[Jens Stutte [:jstutte]]

I assume, that this is stalled or even obsolete for the upcoming work at W3C inside the SW Working Group [1].

[1] https://www.w3.org/2019/11/proposed-sw-wg-charter-2019.html

Comment 158

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 21 votes.
:jjalkanen, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 159

[Andrew Sutherland [:asuth] (he/him)]

Our position on the spec at https://mozilla.github.io/standards-positions/#background-sync is negative so I'm marking this as WONTFIX to clarify the current situation.