Closed Bug 1328517 Opened 3 years ago Closed 3 years ago

DocGroup labeling in /dom/indexeddb

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

(Whiteboard: [QDL][TDC-MVP][DOM])

Attachments

(2 files, 4 obsolete files)

This bug is to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to IndexedDB implementation.
Attached patch (v1) Patch: DocGroup labeling. (obsolete) — Splinter Review
This patch is to label the runnables dispatched to the main thread of the content process.
To achieve this, the major change in IndexedDB is to set the DocGroup specific event target to the actor childs that interact with DOM API of IDBXxx classes:
1. We cache the DocGroup-specific EventTarget or NS_GetCurrentThread() as OwningEventTarget() in IDBFactory once created. OwningEventTarget() method is provided in IDBFactory/IDBDatabase/IDBTransaction for easy access.
2. Set proper event target to the top-level IDB Actor, i.e., IDBFactory to make sure that all the response runnables of top-level actor and its child actors are dispatched to the DocGroup-specific EventTarget.
3. Replace NS_DispatchToMainThread with |mWorkerPrivate->DispatchToMainThread| in which a proper DocGroup-specific EventTarget on main thread has already bound in the constructor of WorkerPrivate. [1] 
4. Override the default event target of WorkerPermissionRequestChildProcessActor/PermissionRequestChildProcessActor to the same one in IDBFactory.
5. Provide DocGroup-specific EventTarget to StreamWrapper to Close()/Destroy() in correct event target.

Wait for treeherder result complete:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b06831fc7194d52048992b3394afd94207414028

[1] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/dom/workers/WorkerPrivate.cpp#4054-4074
Summary: DocGroup labeling in IndexedDB → DocGroup labeling in /dom/indexeddb
Attached patch (v2) Patch: DocGroup labeling. (obsolete) — Splinter Review
Apply nsINamed::SetName() as suggested by the link in comment 0 for more debugging info of the labeled runnables.
Attachment #8826545 - Attachment is obsolete: true
Comment on attachment 8827041 [details] [diff] [review]
(v2) Patch: DocGroup labeling.

Hi Jan,

This is part of the labeling task in Quantom-DOM project:
https://wiki.mozilla.org/Quantum/DOM

The reason for this change was explained at comment 1, and this change was done by reviewing all the *Dispatch* keywords in dom/indexeddb/ to label the runnables dispatched to the main thread of the content process.

Note: the patch change from v1 to v2 is to have nsINamed::SetName() applied as suggested in the wiki for further debugging.

May I have your review for this change?

Thanks!
Attachment #8827041 - Flags: review?(jvarga)
Ok, I'll take a look ASAP.
Comment on attachment 8827041 [details] [diff] [review]
(v2) Patch: DocGroup labeling.

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

r=me, but I'd like to see an updated patch before landing

::: dom/indexedDB/ActorsChild.cpp
@@ +1166,4 @@
>  
>  public:
>    PreprocessHelper(uint32_t aModuleSetIndex, BackgroundRequestChild* aActor)
> +    : mOwningThread(aActor->mTransaction->OwningEventTarget())

It would be nicer if we could get the event target from the actor (since we already set it up for these actors), but it seems there's no method for that right now, only using top level protocol.

@@ +3287,5 @@
>      nsCOMPtr<nsIRunnable> deleteRunnable = new DelayedActionRunnable(
>        this, &BackgroundCursorChild::SendDeleteMeInternal);
> +    MOZ_ALWAYS_SUCCEEDS(
> +      mTransaction->OwningEventTarget()->Dispatch(
> +        deleteRunnable.forget(), NS_DISPATCH_NORMAL));

Here too, it would be nicer to just call GetActorEventTarget().

Nit:
    MOZ_ALWAYS_SUCCEEDS(
      mTransaction->EventTarget()->Dispatch(deleteRunnable.forget(),
                                            NS_DISPATCH_NORMAL));

::: dom/indexedDB/FileSnapshot.cpp
@@ +31,5 @@
>  
>  public:
>    StreamWrapper(nsIInputStream* aInputStream,
> +                IDBFileHandle* aFileHandle,
> +                nsIEventTarget* aOwningEventTarget)

Nit: The constructor for MutableFileBase and other related classes put thread/event target as first argument.

@@ +91,1 @@
>        NewNonOwningRunnableMethod(this, &StreamWrapper::Destroy);

Nit: Add an empty line here.

@@ +91,3 @@
>        NewNonOwningRunnableMethod(this, &StreamWrapper::Destroy);
> +    MOZ_ALWAYS_SUCCEEDS(
> +      destroyRunnable->SetName("StreamWrapper::Destroy"));

One line ?

@@ +171,4 @@
>      return nullptr;
>    }
>  
> +  blobImpl = new BlobImplSnapshot(blobImpl, mFileHandle, mOwningEventTarget);

Not needed I think.

@@ +191,5 @@
>      return;
>    }
>  
> +  RefPtr<StreamWrapper> wrapper =
> +    new StreamWrapper(stream, fileHandle, mOwningEventTarget);

ditto

::: dom/indexedDB/FileSnapshot.h
@@ +45,5 @@
>  
>  public:
>    BlobImplSnapshot(BlobImpl* aImpl,
> +                   IDBFileHandle* aFileHandle,
> +                   nsIEventTarget* aOwningEventTarget);

Not needed I think.

@@ +52,5 @@
>  
>  private:
>    BlobImplSnapshot(BlobImpl* aImpl,
> +                   nsIWeakReference* aFileHandle,
> +                   nsIEventTarget* aOwningEventTarget);

ditto

::: dom/indexedDB/IDBDatabase.cpp
@@ +1041,5 @@
>                              &IDBDatabase::ExpireFileActors,
>                              /* aExpireAll */ false);
>    MOZ_ASSERT(runnable);
> +  MOZ_ALWAYS_SUCCEEDS(
> +    runnable->SetName("IDBDatabase::ExpireFileActors"));

One line ?

@@ +1434,5 @@
>    return NS_OK;
>  }
>  
> +nsIEventTarget*
> +IDBDatabase::OwningEventTarget() const

This should go after AssertIsOnOwningThread().

::: dom/indexedDB/IDBDatabase.h
@@ +296,4 @@
>    virtual JSObject*
>    WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
> +  nsIEventTarget* OwningEventTarget() const;

This should go after AssertIsOnOwningThread().

::: dom/indexedDB/IDBFactory.cpp
@@ +171,5 @@
>    factory->mInnerWindowID = aWindow->WindowID();
>    factory->mPrivateBrowsingMode =
>      loadContext && loadContext->UsePrivateBrowsing();
> +  factory->mOwningEventTarget =
> +    nsGlobalWindow::Cast(aWindow)->EventTargetFor(TaskCategory::Other);

The code here should follow the order of member variables in IDBFactory.h

@@ +302,4 @@
>    factory->mOwningObject = aOwningObject;
>    mozilla::HoldJSObjects(factory.get());
>    factory->mInnerWindowID = aInnerWindowID;
> +  factory->mOwningEventTarget = NS_GetCurrentThread();

Here too.

@@ +893,4 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBFactory)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOwningEventTarget)

Not sure, does this really need to be cycle collected ?

::: dom/indexedDB/IDBFactory.h
@@ +73,5 @@
>    indexedDB::BackgroundFactoryChild* mBackgroundActor;
>  
> +  // A DocGroup-Specific EventTarget if created by CreateForWindow().
> +  // Otherwise, it must be set to NS_GetCurrentThread().
> +  nsCOMPtr<nsIEventTarget> mOwningEventTarget;

Why "Owning" ?

Nit: I would put it after mOwningThread.

@@ +215,4 @@
>    virtual JSObject*
>    WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
> +  nsIEventTarget* OwningEventTarget() const

This should go after AssertIsOnOwningThread().

The return type is on its own line all over this file.

And again, why "Owning" ?

::: dom/indexedDB/IDBMutableFile.cpp
@@ +274,5 @@
>  
>    RefPtr<BlobImpl> blobImplSnapshot =
> +    new BlobImplSnapshot(aBlobImpl,
> +                         static_cast<IDBFileHandle*>(aFileHandle),
> +                         Database()->OwningEventTarget());

I think you can avoid this new extra argument. You can get the event target by:
aFileHandle->GetMutableFile()->Database()->EventTarget()

Or you can add EventTarget() method to IDBMutableFile and IDBFileHandle

::: dom/indexedDB/IDBTransaction.cpp
@@ +1045,5 @@
>    return true;
>  }
>  
> +nsIEventTarget*
> +IDBTransaction::OwningEventTarget() const

This should go after AssertIsOnOwningThread().

::: dom/indexedDB/IDBTransaction.h
@@ +317,5 @@
>    virtual nsresult
>    GetEventTargetParent(EventChainPreVisitor& aVisitor) override;
>  
> +  nsIEventTarget*
> +  OwningEventTarget() const;

This should go after AssertIsOnOwningThread().
Attachment #8827041 - Flags: review?(jvarga) → review+
Comment on attachment 8827041 [details] [diff] [review]
(v2) Patch: DocGroup labeling.

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

::: dom/indexedDB/ActorsChild.cpp
@@ +1166,4 @@
>  
>  public:
>    PreprocessHelper(uint32_t aModuleSetIndex, BackgroundRequestChild* aActor)
> +    : mOwningThread(aActor->mTransaction->OwningEventTarget())

I'll see if I can define a GetActorEventTarget() in ProtocolUtils.cpp for this.

::: dom/indexedDB/IDBFactory.cpp
@@ +893,4 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBFactory)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOwningEventTarget)

It's not necessary. I added it just because its a ref-count object held by IDBFactory.
I'll remove it in next update!

::: dom/indexedDB/IDBFactory.h
@@ +73,5 @@
>    indexedDB::BackgroundFactoryChild* mBackgroundActor;
>  
> +  // A DocGroup-Specific EventTarget if created by CreateForWindow().
> +  // Otherwise, it must be set to NS_GetCurrentThread().
> +  nsCOMPtr<nsIEventTarget> mOwningEventTarget;

Just to have the same naming to mOwningThread that owns this IDBFactory.
I could shorten it as mEventTarget with the getter method named EventTarget().

::: dom/indexedDB/IDBMutableFile.cpp
@@ +274,5 @@
>  
>    RefPtr<BlobImpl> blobImplSnapshot =
> +    new BlobImplSnapshot(aBlobImpl,
> +                         static_cast<IDBFileHandle*>(aFileHandle),
> +                         Database()->OwningEventTarget());

Thanks for capturing this.
Depends on: 1338455
Attached patch (v3) Part 1: DocGroup labeling (obsolete) — Splinter Review
Attachment #8827041 - Attachment is obsolete: true
Depends on: 1339014
1. Rename IDBxxx::OwningEventTarget() to IDBXxx::EventTarget(), and re-order them in .h files.
2. Adopt new IPC Protocol API in bug 1339014 to retrieve EventTarget from actor instead of IDBFactory::EventTarget().
2. Naming a Runnable by its Constructor or its new factory method that accepts a name parameter.
3. Access the event target from a FileHandle without extra parameter from BlobImplSnapshot().
4. Set docgroup event target to BackgroundUtilsChild that was not caught in previous patch.
5. Label ScriptErrorRunnable to SystemGroup.
Attachment #8835896 - Attachment is obsolete: true
Attachment #8837021 - Flags: review?(jvarga)
Attachment #8837021 - Attachment description: 1328517_part1.patch → (v4) Part 1: DocGroup labeling.
This is to have assertion to the event target of each created actor child in the content process to prevent errors like bug 1338455 happened again.
Attachment #8835897 - Attachment is obsolete: true
Attachment #8837022 - Flags: review?(jvarga)
Ok, I'll take a look again.
Comment on attachment 8837021 [details] [diff] [review]
(v4) Part 1: DocGroup labeling.

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +3106,5 @@
>    RefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::Get();
>  
>    if (mgr) {
> +    nsGlobalWindow* inner =
> +      nsGlobalWindow::Cast(window)->GetCurrentInnerWindowInternal();

Hm, I'm not sure if this is right. IndexedDatabaseManager is a service, so there can only be one instance of it and only one instance of BackgroundUtilsChild. nsDOMWindowUtils is created for each nsGlobalWindow.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +996,4 @@
>  
>      BackgroundUtilsChild* actor = new BackgroundUtilsChild(this);
>  
> +    bgActor->SetEventTargetForActor(actor, aEventTarget);

See my comment for nsDOMWindowUtils
Attachment #8837021 - Flags: review?(jvarga) → review+
Comment on attachment 8837022 [details] [diff] [review]
(v4) Part 2: Add Assertion to Each ActorEventTarget.

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

::: dom/indexedDB/ActorsChild.h
@@ +215,4 @@
>    DeallocPBackgroundIDBDatabaseChild(PBackgroundIDBDatabaseChild* aActor)
>                                       override;
>  
> +  virtual mozilla::ipc::IPCResult

Nit: "virtual" is not needed, "override" is enough
Attachment #8837022 - Flags: review?(jvarga) → review+
(In reply to Jan Varga [:janv] from comment #12)
> Hm, I'm not sure if this is right. IndexedDatabaseManager is a service, so
> there can only be one instance of it and only one instance of
> BackgroundUtilsChild. nsDOMWindowUtils is created for each nsGlobalWindow.
>
> > +    bgActor->SetEventTargetForActor(actor, aEventTarget);
> 
> See my comment for nsDOMWindowUtils
Yes, you are right.

The comprehensive way to support labeling is to have a sub-actor per request of GetFileReferences in ipdl to SetEventTargetForActor per request. 
However, after further review, it is not necessary to set EventTarget for this actor because:
1. The GetFileReferences is a sync message in ipdl and the SetEventTargetForActor() is required only for the async response.
2. The DeleteMe is only happened in shutdown phase, so there is no impact without labeling.

I'll rollback the change in nsDOMWindowUtils and IndexedDatabaseManager.

Thanks for pointing out this problem!
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26fd555ab1bb
Part 1: DocGroup labeling. r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/479e6d9edfb7
Part 2: Add Assertion to Each ActorEventTarget. r=janv
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> Yes, you are right.
> 
> The comprehensive way to support labeling is to have a sub-actor per request
> of GetFileReferences in ipdl to SetEventTargetForActor per request. 
> However, after further review, it is not necessary to set EventTarget for
> this actor because:
> 1. The GetFileReferences is a sync message in ipdl and the
> SetEventTargetForActor() is required only for the async response.
> 2. The DeleteMe is only happened in shutdown phase, so there is no impact
> without labeling.
> 
> I'll rollback the change in nsDOMWindowUtils and IndexedDatabaseManager.
> 

Makes sense, thanks.
https://hg.mozilla.org/mozilla-central/rev/26fd555ab1bb
https://hg.mozilla.org/mozilla-central/rev/479e6d9edfb7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Whiteboard: [QDL][TDC-MVP][DOM]
You need to log in before you can comment on or make changes to this bug.