Closed Bug 984990 Opened 6 years ago Closed 6 years ago

Enable MessageChannel for chrome and resource:// callers

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: irakli, Assigned: baku)

References

Details

Attachments

(1 file, 6 obsolete files)

This way devtools team would be able to start using them.
Duplicate of this bug: 984991
I'd like to have Boris' blessing here first.
Flags: needinfo?(bzbarsky)
Summary: Enable MessageChannel for chrome callers → Enable MessageChannel for chrome and resource:// callers
This seems ok to me, as long as what we implement follows the spec and the spec is fairly stable.  Andrea, are we there?
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
(In reply to Boris Zbarsky [:bz] from comment #3)
> This seems ok to me, as long as what we implement follows the spec and the
> spec is fairly stable.  Andrea, are we there?

not yet. currently we have just window-to-window. No worker support for MessagePort yet - bug 911972.
Plus, postMessage() is not following the spec fully - bug 912456
Flags: needinfo?(amarchesini)
Unimplemented parts are OK here as far as I'm concerned.  What I'd like to avoid is chrome callers starting to depend on non-spec behavior and us then breaking them when we start to follow the spec.  Is bug 912456 something that could cause that?
Flags: needinfo?(amarchesini)
Yes. Because the spec wants something like:

mp.postMessage(null, [port])

and we don't support this without that bug fixed.

The same is here:

mp.postMessage({ port: port });

should throw because the transferable object list doesn't contain the port. Here the correct way to write that code:

mp.postMessage({ port: port }, [port]);

Without that bug fixed, the first line doesn't throw.
Flags: needinfo?(amarchesini)
Yeah, that would be a blocker in my book for enabling this for random extensions to use...
(In reply to comment #7)
> Yeah, that would be a blocker in my book for enabling this for random
> extensions to use...

Yes, I think so too.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> (In reply to comment #7)
> > Yeah, that would be a blocker in my book for enabling this for random
> > extensions to use...
> 
> Yes, I think so too.

What do we need in order to move bug 912456 forwards?
Now that bug 912456 is fixed can we move forwards here? Who might be able to work on this?
Flags: needinfo?(ehsan)
I think so, but we should check with Andrea.  Not sure if Andrea has the cycles to work on this or not, I'll let him speak to that effect.  :-)
Flags: needinfo?(ehsan) → needinfo?(amarchesini)
(Oh, and I'm so sorry that I didn't see your comment before, Dave...  I'm close to declaring email bankruptcy.)
This is blocking us from landing some work. I have someone else who might be able to help get this exposed but I really want to hear whether we think we're ready to do that first.
Andrea?
Attached patch chrome.patch (obsolete) — Splinter Review
Boris, just a feedback because I don't know if this is the right approach. Thanks.
Attachment #8423347 - Flags: feedback?(bzbarsky)
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Comment on attachment 8423347 [details] [diff] [review]
chrome.patch

A few issues:

1)  Please don't reinvent xpc::WindowOrNull.
2)  The version of MessageChannel::Enabled that takes an nsPIDOMWindow* doesn't
    need a JSContext*, right?
3)  Checking the document URI seems pretty broken.  Shouldn't this be based on
    the principal?  Why do we care about resource:// exactly?  Seems like this
    should just be done for the IsCallerChrome() cases or something, no?
4)  Why do we need to check enabled in the structured clone bits at all?
Attachment #8423347 - Flags: feedback?(bzbarsky) → feedback+
Attached patch chrome.patch (obsolete) — Splinter Review
Attachment #8423347 - Attachment is obsolete: true
Attachment #8424895 - Flags: review?(bzbarsky)
Comment on attachment 8424895 [details] [diff] [review]
chrome.patch

This doesn't address comment 16 item 3.

Worse yet, this will disable if IsCallerChrome but the URI is not chrome: or resource:, no matter what the pref is set to.  That seems bogus.
Attachment #8424895 - Flags: review?(bzbarsky) → review-
gozala, do you really care about the scheme of the URI or do you want to have MessagePort/MessageChannel enabled for chrome code?
Flags: needinfo?(rFobic)
(In reply to Andrea Marchesini (:baku) from comment #19)
> gozala, do you really care about the scheme of the URI or do you want to
> have MessagePort/MessageChannel enabled for chrome code?

Andrea, it really depends what we mean by enabling for chrome. If that would let chrome code create a message channel and post to a non-chrome privileged document so they could communicate with it, then I'd say it get's 90% of what we need. Only missing peace would be that document we're creating pipeline with won't be able to create ports to send them back, but that can probably wait until message channels are fully enabled.

If chrome code will send message port to a non chrome privileged document, but than that document won't be able to use it to communicate through that port, then enabling only for chrome is not enough I'm afraid.
Flags: needinfo?(rFobic)
Attached patch chrome.patch (obsolete) — Splinter Review
bz, if we want to proceed with this bug, we have to allow chrome and 'resource' scheme url to use MessagePort/MessageChannel. How does it sound?
Attachment #8424895 - Attachment is obsolete: true
Attachment #8433393 - Flags: review?(bzbarsky)
Attached patch chrome.patch (obsolete) — Splinter Review
Attachment #8433393 - Attachment is obsolete: true
Attachment #8433393 - Flags: review?(bzbarsky)
Attachment #8433492 - Flags: review?(bzbarsky)
Comment on attachment 8433492 [details] [diff] [review]
chrome.patch

Just making this chromeonly would handle the 90% case.

But if we do want to expose to resource, we should be basing the check on the principal.  And in particular, we should NOT expose to chrome:// that is not system principal, imo.
Attachment #8433492 - Flags: review?(bzbarsky) → review-
Attached patch chrome.patch (obsolete) — Splinter Review
Here we have the principal check + a mochitest where we have a chrome page loading a html page and sending messages using MessagePort.
Attachment #8433492 - Attachment is obsolete: true
Attachment #8434331 - Flags: review?(bzbarsky)
Comment on attachment 8434331 [details] [diff] [review]
chrome.patch

>+  if (!principal) {

Can't be null.  That's why it has no "Get" prefix.

r=me
Attachment #8434331 - Flags: review?(bzbarsky) → review+
Attached patch chrome.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=23cf03ce4362
Attachment #8434331 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b28cd167b6fe
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.