Get rid of WorkerFeature: WorkerHolder

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8747485 [details] [diff] [review]
features.patch

WorkerFeature is error prone. I want to introduce a WorkerHolder that is able to unregister itself when needed. Plus, 'feature' seems to be the wrong word for what the current implementation does.
Attachment #8747485 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Blocks: 1267907
I'd like to review the cache API changes.

The name change seems a bit unnecessary and creates a lot more churn in this diff.
Flags: needinfo?(bkelly)
Comment on attachment 8747485 [details] [diff] [review]
features.patch

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

I looked through the dom/cache and dom/fetch changes.  They look reasonable.

I'll state again, though, I really don't like the name change.  This is going to cause me a ton of rebasing work in bug 1093357 for what seems very little gain.

::: dom/workers/WorkerHolder.cpp
@@ +15,5 @@
> +}
> +
> +WorkerHolder::~WorkerHolder()
> +{
> +  ReleaseWorkerInternal();

The destructor should assert the expected thread to ensure we're not comparing mWorkerPrivate cross threads.  If some piece of code calls ReleaseWorker() manually its possible they could avoid the thread assertion in ReleaseWorkerInternal.

It would be best to use NS_OWNING_THREAD got this to avoid depending on the possible changing state of mWorkerPrivate.

::: dom/workers/WorkerHolder.h
@@ +68,5 @@
>    // The close handler has run and the worker is effectively dead.
>    Dead
>  };
>  
> +class WorkerHolder

It seems weird to me to make an RAII style class like this, but not define a mechanism for ensuring its cleaned up.  We obviously can't make it a MOZ_STACK_CLASS, but shouldn't this at least declare AddRef()/Release() so implementation classes are ref counted?
Flags: needinfo?(bkelly)
(Assignee)

Comment 3

3 years ago
> I'll state again, though, I really don't like the name change.  This is
> going to cause me a ton of rebasing work in bug 1093357 for what seems very
> little gain.

This patch doesn't have to land before bug 1093357. If you are almost ready to land those patches, this patch can land after them.
More important, a part the rebasing work, do you think 'WorkerHolder' is a better name than 'WorkerFeature'?
Flags: needinfo?(bkelly)
(In reply to Andrea Marchesini (:baku) from comment #3)
> This patch doesn't have to land before bug 1093357. If you are almost ready
> to land those patches, this patch can land after them.
> More important, a part the rebasing work, do you think 'WorkerHolder' is a
> better name than 'WorkerFeature'?

I'm not ready to land, no.  And I understand rebasing comes with the job, but in this case the name change just seems spurious to me.  We could fix the RAII release issue with a much smaller patch.  I don't think "Holder" is appreciably better than "Feature".  WorkerFeature is already in our lexicon.

Also, this name change is from one name to a completely different one.  I can figure out nsRefPtr -> RefPtr, or nsRunnable -> mozilla::Runnable, but its not obvious to me that WorkerFeature -> WorkerHolder.

Anyway, I'll defer to Kyle here.  Just stating my personal opinion.  I'll stop bikeshedding.
Flags: needinfo?(bkelly)
Whiteboard: btpp-active
(Assignee)

Comment 5

3 years ago
Comment on attachment 8747485 [details] [diff] [review]
features.patch

Jonas, if you have time, can you give me a review/feedback about this patch?
Attachment #8747485 - Flags: review?(jonas)
Comment on attachment 8747485 [details] [diff] [review]
features.patch

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

::: dom/workers/WorkerHolder.cpp
@@ +15,5 @@
> +}
> +
> +WorkerHolder::~WorkerHolder()
> +{
> +  ReleaseWorkerInternal();

I agree with this.

::: dom/workers/WorkerHolder.h
@@ +68,5 @@
>    // The close handler has run and the worker is effectively dead.
>    Dead
>  };
>  
> +class WorkerHolder

I don't agree with this. This is a mixin. If bkelly thinks we should add mixin to the class name I'm fine with that, but a mixin shouldn't force reference counting on anything that uses it.

@@ +74,5 @@
>  public:
> +  WorkerHolder();
> +  virtual ~WorkerHolder();
> +
> +  bool KeepAliveWorker(WorkerPrivate* aWorkerPrivate);

How about just HoldWorker.
Attachment #8747485 - Flags: review?(khuey) → review+
(Assignee)

Updated

2 years ago
Attachment #8747485 - Flags: review?(jonas)

Comment 7

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5d78c7ba43
Get rid of WorkerFeature: WorkerHolder, r=khuey
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=30518781&repo=mozilla-inbound
Flags: needinfo?(amarchesini)

Comment 9

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6da60a09547
Backed out changeset 1c5d78c7ba43 for bustage on a CLOSED TREE

Comment 10

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c00c6c0583
Get rid of WorkerFeature: WorkerHolder, r=khuey
(Assignee)

Updated

2 years ago
Flags: needinfo?(amarchesini)

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70c00c6c0583
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Andrea, why did this land without thread assertions in the destructor?  I thought both Kyle and I had asked for that.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 13

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #12)
> Andrea, why did this land without thread assertions in the destructor?  I
> thought both Kyle and I had asked for that.

I did many things rebasing the patch, and I forgot that. I'll do it in a follow up.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Blocks: 1282026
You need to log in before you can comment on or make changes to this bug.