983416 - MessagePort is exposed in workers even though it can't be used for anything

Status: RESOLVED INVALID

(Core :: DOM: Workers, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

Ugh.  We need a test_interfaces for workers...

Can we still fix this before 28 ships?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We're going to respin for the security issues but I don't know that drivers will want to take anything else.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Ugh.  We need a test_interfaces for workers...

Filed bug 983488 for that.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Masatoshi Kimura [:emk]]

Will removing this line work?
https://mxr.mozilla.org/mozilla-central/source/dom/workers/RegisterBindings.cpp?rev=ec85e0c7c060&mark=68-68#68

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Um, so what kind of ports we have enabled. I'm now lost. Bug 952139 is certainly not fixed.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

MessagePorts are required for SharedWorker. We turned them on deliberately, see discussion in bug 924089.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

(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?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

[Lukas Blakk [:lsblakk] use ?needinfo]

(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

[Masatoshi Kimura [:emk]]

(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

[Sylvestre Ledru [:Sylvestre]]

So, we are going to tracking it for 29. Please provide a fix soon for this issue. thanks

Comment 16

[Masatoshi Kimura [:emk]]

28 patch is not provided yet.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Disabling patch

Asking whoever comes first for review.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Did you read comment 8?  Why is this behavior desired?

Comment 19

[Masatoshi Kimura [:emk]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 ...

Comment 21

[Masatoshi Kimura [:emk]]

Comment on attachment 8392187 [details] [diff] [review]
Disabling patch

> but I think this missed the boat ...

Oops.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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

Comment 23

[Masatoshi Kimura [:emk]]

SharedWorker will be enabled in 29, so MessagePort should have a purpose in 29. Maybe we can untrack this bug for 29+.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

OK. Thanks. Tagging also this as unaffected. Don't hesitate to update the 30 flags too.

Comment 25

[Masatoshi Kimura [:emk]]

Untracking and closing as INVALID because MessagePorts are required for SharedWorker since Firefox 29.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Thanks