Implement one-off BackgroundSync API

ASSIGNED
Assigned to

Status

()

Core
DOM: Service Workers
ASSIGNED
2 years ago
19 days ago

People

(Reporter: overholt, Assigned: asuth)

Tracking

(Depends on: 1 bug, Blocks: 6 bugs, {dev-doc-needed})

Trunk
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected)

Details

(URL)

Attachments

(16 attachments, 53 obsolete attachments)

12.43 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
13.27 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
34.20 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
24.85 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
7.73 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
62.85 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.11 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.78 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.86 KB, patch
baku
: review+
Details | Diff | Splinter Review
844 bytes, patch
baku
: review+
Details | Diff | Splinter Review
30.25 KB, patch
baku
: review+
Details | Diff | Splinter Review
51.92 KB, patch
bkelly
: review-
Details | Diff | Splinter Review
28.79 KB, patch
asuth
: review+
Details | Diff | Splinter Review
121.02 KB, patch
Details | Diff | Splinter Review
388.03 KB, patch
Details | Diff | Splinter Review
11.60 KB, patch
baku
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
https://github.com/slightlyoff/BackgroundSync/
I hope you meant after v1.
Depends on: 1173500
No longer depends on: 1059784
(Reporter)

Comment 2

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

Oops, yeah.
Assignee: nobody → ferjmoreno
Summary: Implement BackgroundSync → Implement one-shot BackgroundSync API
Keywords: dev-doc-needed
Summary: Implement one-shot BackgroundSync API → Implement one-off BackgroundSync API
(Assignee)

Comment 3

a year ago
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)
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.
Created attachment 8705632 [details] [diff] [review]
Part 1: WebIDL (WIP)
Created attachment 8705633 [details] [diff] [review]
Part 2: ServiceWorkerRegistration (WIP)
Created attachment 8705634 [details] [diff] [review]
Part 3: IPC (WIP)
(Assignee)

Comment 8

a year ago
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.
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?
(Assignee)

Comment 10

a year ago
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.
I think it will depend on the underlying platform.  Android facilities may differ from whats available on b2g.
(Assignee)

Comment 12

a year ago
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.)
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.
Created attachment 8707035 [details] [diff] [review]
Part 3: IPC (WIP)
Attachment #8705634 - Attachment is obsolete: true
Created attachment 8712697 [details] [diff] [review]
Part 3: IPC (WIP)
Attachment #8707035 - Attachment is obsolete: true
Created attachment 8712699 [details] [diff] [review]
Unified diff (WIP)
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!
Attachment #8712699 - Flags: feedback?(amarchesini)
Created attachment 8712744 [details] [diff] [review]
Part 2: ServiceWorkerRegistration (WIP)

Now it works on workers.
Attachment #8705633 - Attachment is obsolete: true
Created attachment 8712745 [details] [diff] [review]
Part 3: IPC (WIP)
Attachment #8712697 - Attachment is obsolete: true
Created attachment 8712746 [details] [diff] [review]
Unified diff (WIP)
Attachment #8712699 - Attachment is obsolete: true
Attachment #8712699 - Flags: feedback?(amarchesini)
Attachment #8712746 - Flags: feedback?(amarchesini)
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.
Attachment #8712746 - Flags: feedback?(amarchesini) → feedback+

Updated

a year ago
Status: NEW → ASSIGNED
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.
Created attachment 8721385 [details] [diff] [review]
Part 1: WebIDL

Updated with Andrea's feedback addressed. I also took the liberty of alphabetically ordering the entire moz.build.
Attachment #8705632 - Attachment is obsolete: true
Created attachment 8721387 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

Updated with Andrea's feedback.
Attachment #8712744 - Attachment is obsolete: true
Created attachment 8721388 [details] [diff] [review]
Part 3: IPC

Updated with Andrea's feedback.
Attachment #8712745 - Attachment is obsolete: true
Created attachment 8721389 [details] [diff] [review]
Tests
Created attachment 8721408 [details] [diff] [review]
Part 4: SyncService (WIP)
Created attachment 8727182 [details] [diff] [review]
Part 4: SyncEvent
Created attachment 8731368 [details] [diff] [review]
Part 4: SyncEvent
Attachment #8727182 - Attachment is obsolete: true
Created attachment 8731371 [details] [diff] [review]
Part 5: SyncService

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

Next steps:

- Offline/Online logic
- Storage
Attachment #8721408 - Attachment is obsolete: true
Created attachment 8731374 [details] [diff] [review]
Unified diff (WIP)

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

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

Thank you!
Attachment #8712746 - Attachment is obsolete: true
Attachment #8731374 - Flags: feedback?(amarchesini)
Created attachment 8731377 [details]
High level architecture
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 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.
Attachment #8731374 - Flags: feedback?(amarchesini) → feedback+
Created attachment 8733495 [details] [diff] [review]
Part 5: Sync DB
Created attachment 8733497 [details] [diff] [review]
Part 6: SyncService
Attachment #8731371 - Attachment is obsolete: true
Depends on: 1260138
Depends on: 1260141
Blocks: 1260138, 1260141
No longer depends on: 1260138, 1260141
Created attachment 8736435 [details] [diff] [review]
Unified diff (WIP)

This patch addresses your previous feedback and adds:

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

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

Thank you!
Attachment #8731374 - Attachment is obsolete: true
Attachment #8736435 - Flags: feedback?(amarchesini)
Created attachment 8736438 [details] [diff] [review]
Unified diff (WIP)

And now with the correct patch. Sorry for the noise.
Attachment #8736435 - Attachment is obsolete: true
Attachment #8736435 - Flags: feedback?(amarchesini)
Attachment #8736438 - Flags: feedback?(amarchesini)
Attachment #8736438 - Flags: feedback?(amarchesini)
Attachment #8721385 - Attachment is obsolete: true
Attachment #8721387 - Attachment is obsolete: true
Attachment #8721388 - Attachment is obsolete: true
Attachment #8721389 - Attachment is obsolete: true
Attachment #8731368 - Attachment is obsolete: true
Attachment #8733495 - Attachment is obsolete: true
Attachment #8733497 - Attachment is obsolete: true
Attachment #8736438 - Attachment is obsolete: true
Created attachment 8743357 [details] [diff] [review]
Part 1: WebIDL and SyncManager
Attachment #8743357 - Flags: review?(amarchesini)
Created attachment 8743359 [details] [diff] [review]
Part 2: ServiceWorkerRegistration
Attachment #8743359 - Flags: review?(amarchesini)
Created attachment 8743360 [details] [diff] [review]
Part 3: IPC
Attachment #8743360 - Flags: review?(amarchesini)
Created attachment 8743362 [details] [diff] [review]
Part 4: SyncEvent
Attachment #8743362 - Flags: review?(amarchesini)
Created attachment 8743363 [details] [diff] [review]
Part 5: Sync DB
Attachment #8743363 - Flags: review?(amarchesini)
Created attachment 8743364 [details] [diff] [review]
Part 6: SyncService
Attachment #8743364 - Flags: review?(amarchesini)
Created attachment 8743365 [details] [diff] [review]
Part 7: Tests
Attachment #8743365 - Flags: review?(amarchesini)
Created attachment 8743366 [details] [diff] [review]
Unified diff
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.
Attachment #8743357 - Flags: review?(amarchesini) → review+
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.
Attachment #8743359 - Flags: review?(amarchesini) → review+
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<..
Attachment #8743360 - Flags: review?(amarchesini) → review+
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.
Attachment #8743362 - Flags: review?(amarchesini) → review+
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.
Attachment #8743363 - Flags: review?(amarchesini)
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
Attachment #8743364 - Flags: review?(amarchesini)
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.
Attachment #8743365 - Flags: review?(amarchesini) → review+

Updated

11 months ago
Blocks: 1273644
Created attachment 8759773 [details] [diff] [review]
Unified diff

Unified diff with the WIP refactor
Attachment #8743366 - Attachment is obsolete: true
Created attachment 8763785 [details] [diff] [review]
Part 1: WebIDL and DOM bindings
Attachment #8743357 - Attachment is obsolete: true
Attachment #8763785 - Flags: review?(amarchesini)
Created attachment 8763788 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

This one has not changed except for the s/Sync/BackgroundSync and the review feedback from comment 48. So keeping the r+.
Attachment #8743359 - Attachment is obsolete: true
Attachment #8763788 - Flags: review+
Created attachment 8763790 [details] [diff] [review]
Part 3: IPC
Attachment #8743360 - Attachment is obsolete: true
Attachment #8763790 - Flags: review?(amarchesini)
Created attachment 8763791 [details] [diff] [review]
Part 4: Sync event

This one didn't changed much apart from the s/Sync/BackgroundSync change so taking r+ from comment 50.
Attachment #8743362 - Attachment is obsolete: true
Attachment #8763791 - Flags: review+
Created attachment 8763794 [details] [diff] [review]
Part 5: Storage

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

I took a significant part of this code from the Cache API, as we have similar requirements. At some point we may want to abstract and move some of this code to a common helper.
Attachment #8743363 - Attachment is obsolete: true
Attachment #8763794 - Flags: review?(amarchesini)
Created attachment 8763795 [details] [diff] [review]
Part 6: OnlineObserver
Attachment #8743364 - Attachment is obsolete: true
Attachment #8763795 - Flags: review?(amarchesini)
Created attachment 8763796 [details] [diff] [review]
Part 7: BackgroundSyncService
Attachment #8763796 - Flags: review?(amarchesini)
Created attachment 8763797 [details] [diff] [review]
Part 8: Tests
Attachment #8743365 - Attachment is obsolete: true
Attachment #8763797 - Flags: review?(amarchesini)
Attachment #8731377 - Attachment is obsolete: true
Created attachment 8763799 [details] [diff] [review]
Unified diff
Attachment #8759773 - Attachment is obsolete: true

Updated

10 months ago
Attachment #8763785 - Flags: review?(amarchesini) → review+
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.
Attachment #8763790 - Flags: review?(amarchesini) → review+
I gave you r+, but I want to see a interdiff with WorkerFeature to WorkerHolder.
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
Attachment #8763795 - Flags: review?(amarchesini) → review+
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);
Attachment #8763796 - Flags: review?(amarchesini) → review-
Attachment #8763797 - Flags: review?(amarchesini) → review+
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 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.
Attachment #8763794 - Flags: review?(amarchesini) → review?(bugmail)
(Assignee)

Comment 70

9 months ago
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?
Attachment #8763794 - Flags: feedback?(bkelly)
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?
Flags: needinfo?(ferjmoreno)
Attachment #8763794 - Flags: feedback?(bkelly)
(Assignee)

Comment 72

9 months ago
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)
Attachment #8763794 - Flags: review?(bugmail)
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.
Flags: needinfo?(ferjmoreno)
(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!
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.
Created attachment 8777369 [details] [diff] [review]
Part 1: WebIDL and DOM bindings

Rebased on top of latest m-i. Keeping baku's r+ after comment 63
Attachment #8763785 - Attachment is obsolete: true
Attachment #8777369 - Flags: review+
Created attachment 8777370 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

Rebased as well. r+ from comment 48.
Attachment #8763788 - Attachment is obsolete: true
Attachment #8777370 - Flags: review+
Created attachment 8777373 [details] [diff] [review]
Part 3: IPC

Addressed Andrea's feedback from comment 64. Interdiff with WorkerFeature to WorkerHolder on the way.
Attachment #8763790 - Attachment is obsolete: true
Attachment #8777373 - Flags: review+
Created attachment 8777386 [details] [diff] [review]
Part 3.1. : Interdiff with WorkerFeature to WorkerHolder

Interdiff with WorkerFeature to WorkerHolder as requested in comment 65
Attachment #8777386 - Flags: review?(amarchesini)
Created attachment 8777388 [details] [diff] [review]
Part 4: Sync event

Rebased. r+ from comment 50
Attachment #8763791 - Attachment is obsolete: true
Created attachment 8777390 [details] [diff] [review]
Part 6: OnlineObserver

Addressed Andrea's feedback. r+ from comment 66.
Attachment #8763795 - Attachment is obsolete: true
Attachment #8777390 - Flags: review+
Comment on attachment 8777388 [details] [diff] [review]
Part 4: Sync event

r+ from comment 50 (now for real)
Attachment #8777388 - Flags: review+
Created attachment 8777844 [details] [diff] [review]
Part 7: BackgroundSyncService

Feedback from comment 67 addressed
Attachment #8763796 - Attachment is obsolete: true
(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.
Attachment #8777386 - Flags: review?(amarchesini) → review+
Created attachment 8788555 [details] [diff] [review]
Part 1: WebIDL and DOM bindings

Rebased on top of m-i.

r+ from comment 63.
Attachment #8763794 - Attachment is obsolete: true
Attachment #8763797 - Attachment is obsolete: true
Attachment #8763799 - Attachment is obsolete: true
Attachment #8777369 - Attachment is obsolete: true
Attachment #8777370 - Attachment is obsolete: true
Attachment #8777373 - Attachment is obsolete: true
Attachment #8777386 - Attachment is obsolete: true
Attachment #8777388 - Attachment is obsolete: true
Attachment #8777390 - Attachment is obsolete: true
Attachment #8777844 - Attachment is obsolete: true
Attachment #8788555 - Flags: review+
Created attachment 8788558 [details] [diff] [review]
Part 2: ServiceWorkerRegistration

r+ from comment 48.
Attachment #8788558 - Flags: review+
Created attachment 8788560 [details] [diff] [review]
Part 3: IPC

r+ from comment 64
Attachment #8788560 - Flags: review+
Created attachment 8788562 [details] [diff] [review]
Part 4: Sync event

r+ from comment 50.
Attachment #8788562 - Flags: review+
Created attachment 8788567 [details] [diff] [review]
Part 5: QuotaClient helpers

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

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

In this patch I won't change the dom/cache implementation as I'd like to do that in a different bug, so this one can land independently.
Attachment #8788567 - Flags: feedback?(bkelly)
Created attachment 8788569 [details] [diff] [review]
Part 6: Storage

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

For this API we store a list of sync registrations per each origin and a list of the origins that has any sync registration stored. The former is handled by StorageManager*, the later by ChromeStorageManager*.
Attachment #8788569 - Flags: feedback?(bkelly)
Created attachment 8788571 [details] [diff] [review]
Part 8: OnlineObserver

r+ from comment 66.
Attachment #8788571 - Flags: review+
(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.
Created attachment 8788574 [details] [diff] [review]
Part 9: BackgroundSyncService

Rebased and addresses feedback from comment 67
Attachment #8788574 - Flags: review?(amarchesini)
Created attachment 8788575 [details] [diff] [review]
Tests

r+ from comment 68.
Attachment #8788575 - Flags: review+
Created attachment 8788576 [details] [diff] [review]
Unified diff
(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.
Blocks: 1300844
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.
Attachment #8788567 - Flags: feedback?(jvarga)
Attachment #8788567 - Flags: feedback?(bkelly)
Attachment #8788567 - Flags: feedback+
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.
Attachment #8788569 - Flags: feedback?(bkelly) → feedback-

Comment 99

7 months ago
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.
Attachment #8788567 - Flags: feedback?(jvarga)
(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.
Flags: needinfo?(jvarga)

Comment 101

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

Comment 102

7 months ago
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

7 months ago
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

7 months ago
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

7 months ago
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 ?
Flags: needinfo?(jvarga)
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)

Comment 108

7 months ago
I'm ok with something like dom/cache/something or if that's not acceptable dom/quota/something.
I'll think about suitable name.
dom/quota/shared or dom/quota/util would be my vote.

Comment 110

7 months ago
dom/quota/shared sounds good
Blocks: 1308447
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.
Attachment #8788574 - Flags: review?(amarchesini) → review+
Depends on: 1226983
No longer depends on: 1173500
Attachment #8788555 - Attachment is obsolete: true
Attachment #8788558 - Attachment is obsolete: true
Attachment #8788560 - Attachment is obsolete: true
Attachment #8788562 - Attachment is obsolete: true
Attachment #8788567 - Attachment is obsolete: true
Attachment #8788569 - Attachment is obsolete: true
Attachment #8788555 - Attachment is obsolete: false
Attachment #8788558 - Attachment is obsolete: false
Attachment #8788560 - Attachment is obsolete: false
Attachment #8788562 - Attachment is obsolete: false
Created attachment 8832151 [details] [diff] [review]
Part 1.1 (interdiff): WebIDL and DOM bindings
Attachment #8832151 - Flags: review?(amarchesini)
Created attachment 8832152 [details] [diff] [review]
Part 3.1 (interdiff): IPC
Attachment #8832152 - Flags: review?(amarchesini)
Created attachment 8832153 [details] [diff] [review]
Part 4.1 (interdiff): Sync event
Attachment #8832153 - Flags: review?(amarchesini)
Created attachment 8832154 [details] [diff] [review]
Part 8.1 (interdiff): OnlineStateObserver
Attachment #8832154 - Flags: review?(amarchesini)
Attachment #8788571 - Attachment description: Part 7: OnlineObserver → Part 8: OnlineObserver
Attachment #8788574 - Attachment description: Part 8: BackgroundSyncService → Part 9: BackgroundSyncService
Attachment #8788575 - Attachment description: Part 9: Tests → Tests
Created attachment 8832155 [details] [diff] [review]
Part 9.1 (interdiff): BackgroundSyncService
Attachment #8832155 - Flags: review?(amarchesini)
Created attachment 8832156 [details] [diff] [review]
Part 5: QuotaClient helpers
Created attachment 8832157 [details] [diff] [review]
Part 6: mozStorageConnectionUtils
Created attachment 8832158 [details] [diff] [review]
Part 7: Storage
Created attachment 8832159 [details] [diff] [review]
Unified diff
Attachment #8788576 - Attachment is obsolete: true
Created attachment 8832167 [details] [diff] [review]
Tests (interdiff)
Attachment #8832167 - Flags: review?(amarchesini)
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.
Attachment #8832156 - Flags: review?(bkelly)
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.
Attachment #8832157 - Flags: review?(bkelly)
Attachment #8832158 - Flags: review?(bkelly)
Created attachment 8832450 [details] [diff] [review]
Tests
Attachment #8788575 - Attachment is obsolete: true
Attachment #8832167 - Attachment is obsolete: true
Attachment #8832167 - Flags: review?(amarchesini)
Attachment #8832450 - Flags: review?(amarchesini)
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
Attachment #8832156 - Flags: review?(bkelly) → review-
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.
Attachment #8832157 - Flags: review?(bkelly) → review?(bugmail)
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!
Attachment #8832158 - Flags: review?(bkelly)
(Assignee)

Comment 128

3 months ago
(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!
(Assignee)

Comment 129

2 months ago
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
Attachment #8832157 - Flags: review?(bugmail)
Attachment #8832157 - Flags: review+
Attachment #8832157 - Flags: feedback?(mak77)
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.
Attachment #8832157 - Flags: feedback?(mak77)
(Assignee)

Comment 131

2 months ago
(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.
(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.
Attachment #8832151 - Flags: review?(amarchesini) → review+
Attachment #8832152 - Flags: review?(amarchesini) → review+
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.
Attachment #8832153 - Flags: review?(amarchesini) → review+
Attachment #8832154 - Flags: review?(amarchesini) → review+
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.
Attachment #8832155 - Flags: review?(amarchesini) → review+
Attachment #8832450 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 135

25 days ago
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!
Assignee: ferjmoreno → bugmail
Thanks for following up with this work Andrew. Yes, what's attached to the bug contains the current state of the patch.
You need to log in before you can comment on or make changes to this bug.