Closed
Bug 983416
Opened 11 years ago
Closed 11 years ago
MessagePort is exposed in workers even though it can't be used for anything
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
firefox28 | - | wontfix |
firefox29 | - | unaffected |
firefox30 | --- | unaffected |
People
(Reporter: smaug, Unassigned)
References
Details
Attachments
(1 file)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
In FF27 '"MessagePort" in this' and '"WorkerMessagePort"' in this are both
false in worker scope.
'"MessagePort" in this' is true in FF28.
Same for window scope.
I guess MessagePort should have been [NoInterfaceObject] until it is actually enabled everywhere.
Comment 1•11 years ago
|
||
Ugh. We need a test_interfaces for workers...
Can we still fix this before 28 ships?
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
Flags: needinfo?(amarchesini)
We're going to respin for the security issues but I don't know that drivers will want to take anything else.
Comment 3•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1)
> Ugh. We need a test_interfaces for workers...
Filed bug 983488 for that.
Comment 4•11 years ago
|
||
what's the user impact here? can we revert to a known-good state? as we are currently preparing to respin for security issues we could consider a low risk backout.
Flags: needinfo?(bugs)
If it's even possible, backing out bug 928312, which is a massive refactoring of this subsystem, would introduce a tremendous amount of risk. We could take a vary narrowly tailored patch to fix just this issue though.
Comment 6•11 years ago
|
||
Will removing this line work?
https://mxr.mozilla.org/mozilla-central/source/dom/workers/RegisterBindings.cpp?rev=ec85e0c7c060&mark=68-68#68
Unclear. We do have a message port in workers, the message port that comes from a SharedWorker. We'd need to test that that still behaves as expected.
Comment 8•11 years ago
|
||
Removing that one line will remove the global's "MessagePort" property... by default.
As soon as a MessagePort object is actually constructed in the worker, this.MessagePort will get defined in global scope. Are we ok with that?
If not, we'd need to throw in a [Func] or [NoInterfaceObject] on the interface if we really want to hide it. Though given that we do have MessagePorts around, it's not clear to me why we'd want to hide it exactly.
Reporter | ||
Comment 9•11 years ago
|
||
Um, so what kind of ports we have enabled. I'm now lost. Bug 952139 is certainly not fixed.
Flags: needinfo?(bugs)
MessagePorts are required for SharedWorker. We turned them on deliberately, see discussion in bug 924089.
Comment 11•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10)
> MessagePorts are required for SharedWorker. We turned them on deliberately,
> see discussion in bug 924089.
So that's for 29 - they aren't needed in FF28?
If SharedWorker was turned on by default in 29 then we only need MessagePort in 29 I believe.
We shouldn't need it in 28 unless things somehow go haywire if someone had flipped the pref in their own profile...
Comment 13•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> If SharedWorker was turned on by default in 29 then we only need MessagePort
> in 29 I believe.
>
> We shouldn't need it in 28 unless things somehow go haywire if someone had
> flipped the pref in their own profile...
We don't tend towards blocking/tracking an issue that is affected by users flipping a pref that is not on by default. Unless there's a user impact here for the default setting of FF28 with regards to MessagePorts, I don't see anything to track here.
Comment 14•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> We shouldn't need it in 28 unless things somehow go haywire if someone had
> flipped the pref in their own profile...
Then is comment #6 safe after all?
(In reply to Lukas Blakk [:lsblakk] from comment #13)
> We don't tend towards blocking/tracking an issue that is affected by users
> flipping a pref that is not on by default. Unless there's a user impact
> here for the default setting of FF28 with regards to MessagePorts, I don't
> see anything to track here.
SharedWorker is not enabled in 28, but MessagePort is. The latter is the problem. Fortunately, we can safely disable MessagePort in 28 because SharedWorker is not enabled in the release yet.
Comment 15•11 years ago
|
||
So, we are going to tracking it for 29. Please provide a fix soon for this issue. thanks
Comment 16•11 years ago
|
||
28 patch is not provided yet.
Comment 17•11 years ago
|
||
Asking whoever comes first for review.
Attachment #8392187 -
Flags: review?(khuey)
Attachment #8392187 -
Flags: review?(bent.mozilla)
Did you read comment 8? Why is this behavior desired?
Flags: needinfo?(VYV03354)
Comment 19•11 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18)
> Did you read comment 8? Why is this behavior desired?
According to the following comments, SharedWorker is disabled in 28. Who is constructing MessagePort?
Flags: needinfo?(VYV03354)
Comment on attachment 8392187 [details] [diff] [review]
Disabling patch
Review of attachment 8392187 [details] [diff] [review]:
-----------------------------------------------------------------
Ok. r+, but I think this missed the boat ...
Attachment #8392187 -
Flags: review?(khuey) → review+
Comment 21•11 years ago
|
||
Comment on attachment 8392187 [details] [diff] [review]
Disabling patch
> but I think this missed the boat ...
Oops.
Attachment #8392187 -
Flags: review?(bent.mozilla)
Updated•11 years ago
|
Updated•11 years ago
|
Flags: needinfo?(amarchesini)
Comment 22•11 years ago
|
||
Masatoshi, there is the 29 boat ;) Can you make sure the patch land in m-c and fill the necessary uplift requests to aurora and beta? Thanks
Flags: needinfo?(VYV03354)
Comment 23•11 years ago
|
||
SharedWorker will be enabled in 29, so MessagePort should have a purpose in 29. Maybe we can untrack this bug for 29+.
Flags: needinfo?(VYV03354)
Comment 24•11 years ago
|
||
OK. Thanks. Tagging also this as unaffected. Don't hesitate to update the 30 flags too.
Comment 25•11 years ago
|
||
Untracking and closing as INVALID because MessagePorts are required for SharedWorker since Firefox 29.
Comment 26•11 years ago
|
||
Thanks
You need to log in
before you can comment on or make changes to this bug.
Description
•