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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
firefox28 - wontfix
firefox29 - unaffected
firefox30 --- unaffected

People

(Reporter: smaug, Unassigned)

References

Details

Attachments

(1 file)

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.
Ugh. We need a test_interfaces for workers... Can we still fix this before 28 ships?
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.
(In reply to Boris Zbarsky [:bz] from comment #1) > Ugh. We need a test_interfaces for workers... Filed bug 983488 for that.
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.
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.
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.
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.
(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...
(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.
(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.
So, we are going to tracking it for 29. Please provide a fix soon for this issue. thanks
28 patch is not provided yet.
Attached patch Disabling patch (deleted) — Splinter Review
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)
(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 on attachment 8392187 [details] [diff] [review] Disabling patch > but I think this missed the boat ... Oops.
Attachment #8392187 - Flags: review?(bent.mozilla)
Flags: needinfo?(amarchesini)
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)
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)
OK. Thanks. Tagging also this as unaffected. Don't hesitate to update the 30 flags too.
Untracking and closing as INVALID because MessagePorts are required for SharedWorker since Firefox 29.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: