If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Label dom/filesystem

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: smaug, Assigned: baku)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
baku, could you take a look?

Bug 1321812 has the background information.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

7 months ago
Created attachment 8852042 [details] [diff] [review]
part 1 - Runnable
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8852042 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

7 months ago
Created attachment 8852044 [details] [diff] [review]
part 2 - DispatchTo{Current,Main}Thread
Attachment #8852044 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

7 months ago
Created attachment 8852046 [details] [diff] [review]
part 3 - Actor
Attachment #8852046 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

7 months ago
Created attachment 8852050 [details] [diff] [review]
part 3 - actor

Note that I cannot call SetEventTargetForActor() if the API is used in workers otherwise I receive a MOZ_CRASH() complaining that the workerID doesn't match with the ID of the IPC message.
Attachment #8852046 - Attachment is obsolete: true
Attachment #8852046 - Flags: review?(wmccloskey)
Attachment #8852050 - Flags: review?(wmccloskey)

Updated

7 months ago
See Also: → bug 1331568

Updated

7 months ago
See Also: bug 1331568

Updated

7 months ago
Attachment #8852042 - Flags: review?(wmccloskey) → review+
Comment on attachment 8852044 [details] [diff] [review]
part 2 - DispatchTo{Current,Main}Thread

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

There's a lot of duplication of the GetParentObject/EventTargetFor/Dispatch code. Can you instead factor it into a common function or something? Or maybe a method on FileSystemEntry (although that wouldn't cover everything)?

::: dom/filesystem/compat/FileSystemDirectoryReader.cpp
@@ +103,5 @@
> +      nsIEventTarget* target =
> +        mParentEntry->GetParentObject()->EventTargetFor(TaskCategory::Other);
> +      MOZ_ASSERT(target);
> +
> +      target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

You're missing the NS_WARNING_ASSERTION here.

@@ +167,5 @@
> +    nsIEventTarget* target =
> +      GetParentObject()->EventTargetFor(TaskCategory::Other);
> +    MOZ_ASSERT(target);
> +
> +    target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

Same.

::: dom/filesystem/compat/FileSystemRootDirectoryEntry.cpp
@@ +118,5 @@
> +      nsIEventTarget* target =
> +        GetParentObject()->EventTargetFor(TaskCategory::Other);
> +      MOZ_ASSERT(target);
> +
> +      target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

You're missing the NS_WARNING_ASSERTION here.
Attachment #8852044 - Flags: review?(wmccloskey) → review+
Comment on attachment 8852050 [details] [diff] [review]
part 3 - actor

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

::: dom/filesystem/FileSystemTaskBase.cpp
@@ +167,4 @@
>    if (HasError()) {
>      // In this case we don't want to use IPC at all.
>      RefPtr<ErrorRunnable> runnable = new ErrorRunnable(this);
> +    target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

NS_WARNING_ASSERTION

@@ +194,5 @@
>      mozilla::ipc::BackgroundChild::GetForCurrentThread();
>    MOZ_ASSERT(actor);
>  
> +  if (NS_IsMainThread()) {
> +    aActor->SetEventTargetForActor(this, target);

This confused me. Is |aActor| always equal to |actor|? Can we just get rid of |actor|?

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +51,5 @@
>  
>    return task.forget();
>  }
>  
> +GetFileOrDirectoryTaskChild::GetFileOrDirectoryTaskChild(nsIGlobalObject* aGlobalObject, 

Extra space at the end of the line.
Attachment #8852050 - Flags: review?(wmccloskey) → review+

Comment 8

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0d8e859a64
Labeling dom/filesystem - part 1 - Runnable, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/611184e06d80
Labeling dom/filesystem - part 2 - DispatchTo{Current,Main}Thread, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a269457f31c9
Labeling dom/filesystem - part 3 - IPC actors, r=billm

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9d0d8e859a64
https://hg.mozilla.org/mozilla-central/rev/611184e06d80
https://hg.mozilla.org/mozilla-central/rev/a269457f31c9
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.