Ensure that the registration of empty service workers succeeds

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → ehsan
Comment on attachment 8585204 [details] [diff] [review]
Ensure that the registration of empty service workers succeeds

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

Since this also affects normal workers, do you want to run it by a worker peer? I couldn't find anything in the html spec that prohibits empty workers.
Attachment #8585204 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8585204 [details] [diff] [review]
Ensure that the registration of empty service workers succeeds

Ben, what do you think?  We shouldn't fail creating workers from empty script files, right?
Attachment #8585204 - Flags: review?(bent.mozilla)
ping?
Flags: needinfo?(bent.mozilla)
Comment on attachment 8585204 [details] [diff] [review]
Ensure that the registration of empty service workers succeeds

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

Hrm... I don't know about this. Why do we want to make it "ok" to create useless workers? They will take up memory, tie up a thread, etc.

If this happens some developer has clearly screwed up, right? Seems like we should at least spit something into the error console?

What's the motivation for this change?
Attachment #8585204 - Flags: review?(bent.mozilla)
Flags: needinfo?(bent.mozilla)
Attachment #8585204 - Flags: review-
I find it very surprising that we're more stringent on accepting empty worker scripts than main-thread empty scripts using the <script> tag.  I originally caught this bug when I was starting to work on a small app using service workers and I noticed that the registration was failing as I was working with that code because I started off with an empty file for a service worker.

Is there a good reason to break this kind of write/test/debug cycle?

Also, for whatever it's worth, what we have now violates the worker spec, since "" is valid JS code.
Flags: needinfo?(bent.mozilla)
Well, main thread empty scripts don't seem much less costly than empty worker scripts, at least in our current code... I'm fine with us fixing this, but we should fix it correctly (warning in the error console, not hogging a thread until the worker is GC'd).
Flags: needinfo?(bent.mozilla)
The warning part is easy, but I don't know how to selectively create or not create threads for workers depending on their script source code, and I don't know what we would consider an "empty" script (is it ""?  " "? "\t"? "/* comment */"? "function YouCantCallMe(){...}"?)

So I guess I'll drop this.
Assignee: ehsan → nobody
We could probably just have a special hook for IsEmpty(), or Trim().IsEmpty(). I bet all you have to do is call WorkerPrivate::Close() to get it to drop the thread.
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #9)
> We could probably just have a special hook for IsEmpty(), or
> Trim().IsEmpty(). I bet all you have to do is call WorkerPrivate::Close() to
> get it to drop the thread.

That seems to be too soon, since it makes it impossible to post new runnables to the worker, which fails the sending of the LifecycleEventWorkerRunnable to the service worker, which causes the Promise returned from register() to never resolve or reject.

Any other ideas?
Flags: needinfo?(bent.mozilla)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> That seems to be too soon, since it makes it impossible to post new
> runnables to the worker, which fails the sending of the
> LifecycleEventWorkerRunnable to the service worker, which causes the Promise
> returned from register() to never resolve or reject.

That sounds like a bug... There's nothing preventing the worker script to call close() directly, and it sounds like that hoses us. nsm?

> Any other ideas?

Well, if we fix that above, it should be ok right? I mean, if the worker is empty do we need to do any lifecycle stuff or further initialization?
Flags: needinfo?(bent.mozilla) → needinfo?(nsm.nikhil)
Sorry ehsan, see comment 11
Flags: needinfo?(ehsan)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > That seems to be too soon, since it makes it impossible to post new
> > runnables to the worker, which fails the sending of the
> > LifecycleEventWorkerRunnable to the service worker, which causes the Promise
> > returned from register() to never resolve or reject.
> 
> That sounds like a bug... There's nothing preventing the worker script to
> call close() directly, and it sounds like that hoses us. nsm?

Yes, there is.  :-)  <https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#178>

> > Any other ideas?
> 
> Well, if we fix that above, it should be ok right?

Given that per spec, service workers cannot call close on themselves, there's no problem to be fixed unless if we start calling WorkerPrivate::Close ourselves.

> I mean, if the worker is
> empty do we need to do any lifecycle stuff or further initialization?

No, but there is a certain level of stuff that I think is justified to ripple out through the code just to be able to not have OS level threads for empty workers.  AFAICT what you're asking is not easily implementable, so if I do have to make the choice of violating the spec for empty workers and have the registration of those be broken vs have the entire SW lifecycle code be aware of this special case, I would take the former.  I'll leave Nikhil's needinfo so he can respond as well.

(I do think however that we are overthinking this, and it's not worth it to try to close the worker private here.)
Flags: needinfo?(ehsan)
Comment on attachment 8585204 [details] [diff] [review]
Ensure that the registration of empty service workers succeeds

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

Sigh. Since it's so much work to fix this better I guess it's ok to do this.

I would love a console warning though! Seems like this is not something a sane web dev would try to do :)
Attachment #8585204 - Flags: review- → review+
Thanks.  I will land with a console warning.
Flags: needinfo?(nsm.nikhil)
Assignee: nobody → ehsan
https://hg.mozilla.org/mozilla-central/rev/9956e7b57de9
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.