DocGroup labeling in /dom/indexeddb

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

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

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

11 months ago
This bug is to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to IndexedDB implementation.
(Assignee)

Comment 1

10 months ago
Created attachment 8826545 [details] [diff] [review]
(v1) Patch: DocGroup labeling.

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
(Assignee)

Updated

10 months ago
Summary: DocGroup labeling in IndexedDB → DocGroup labeling in /dom/indexeddb
(Assignee)

Comment 2

10 months ago
Created attachment 8827041 [details] [diff] [review]
(v2) Patch: DocGroup labeling.

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
(Assignee)

Comment 3

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

Comment 4

10 months ago
Ok, I'll take a look ASAP.

Comment 5

10 months ago
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+
(Assignee)

Comment 6

10 months ago
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.
(Assignee)

Updated

10 months ago
Depends on: 1338455
(Assignee)

Comment 7

10 months ago
Created attachment 8835896 [details] [diff] [review]
(v3) Part 1: DocGroup labeling
Attachment #8827041 - Attachment is obsolete: true
(Assignee)

Comment 8

10 months ago
Created attachment 8835897 [details] [diff] [review]
(WIP) Part 2: Add Assertion to Each ActorEventTarget.
(Assignee)

Updated

9 months ago
Depends on: 1339014
(Assignee)

Comment 9

9 months ago
Created attachment 8837021 [details] [diff] [review]
(v4) Part 1: DocGroup labeling.

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)
(Assignee)

Updated

9 months ago
Attachment #8837021 - Attachment description: 1328517_part1.patch → (v4) Part 1: DocGroup labeling.
(Assignee)

Comment 10

9 months ago
Created attachment 8837022 [details] [diff] [review]
(v4) Part 2: Add Assertion to Each ActorEventTarget.

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)

Comment 11

9 months ago
Ok, I'll take a look again.

Comment 12

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

9 months ago
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+
(Assignee)

Comment 14

9 months ago
(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!

Comment 15

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

Comment 16

9 months ago
(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.

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26fd555ab1bb
https://hg.mozilla.org/mozilla-central/rev/479e6d9edfb7
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

8 months ago
Whiteboard: [QDL][TDC-MVP][DOM]
You need to log in before you can comment on or make changes to this bug.