Closed Bug 966439 Opened 10 years ago Closed 9 years ago

Implement BroadcastChannel API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
relnote-firefox --- 38+

People

(Reporter: sicking, Assigned: baku)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(11 files, 38 obsolete files)

8.16 KB, patch
Details | Diff | Splinter Review
54.10 KB, patch
Details | Diff | Splinter Review
23.04 KB, patch
Details | Diff | Splinter Review
6.54 KB, patch
Details | Diff | Splinter Review
6.98 KB, patch
Details | Diff | Splinter Review
25.33 KB, patch
Details | Diff | Splinter Review
19.66 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
14.68 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.78 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
2.30 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
2.34 KB, patch
Details | Diff | Splinter Review
Spec at

http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#broadcasting-to-other-browsing-contexts

This allows all windows and workers from the same Origin to talk to each other, even if they don't have a direct reference to each other.

We should make sure we make it work in e10s as well. Though keep in mind that we should use extended origins (appid+browserflag+origin) when determining who can talk to who.
Oh, and there appears to be a bug in the current spec. BroadcastChannel should inherit EventTarget.
Oh, and we need to make sure that a child process can't get broadcastchannel notifications for an appid that it's not running.
Attached patch patch 1 - MessageEvent (obsolete) — Splinter Review
Attachment #8407600 - Flags: review?(bugs)
Attachment #8407602 - Flags: review?(bugs)
The next patches is for IPC and workers. They are both written but I have to fix a couple of life-time issue.
OS: Mac OS X → All
Hardware: x86 → All
Andrea, can you please send an intent to ship email to dev-platform for this?  Thanks!
Note, I think we should get MessagePort/Channel working first everywhere, before enabling this
(especially the non-spec'ed parts of this).
Keywords: dev-doc-needed
(In reply to Olli Pettay [:smaug] from comment #7)
> Note, I think we should get MessagePort/Channel working first everywhere,
> before enabling this
> (especially the non-spec'ed parts of this).

Why?  This is not yet exposed to workers right?
Ah, right. This part is actually spec'ed. window-window is fine.
A better management of the origin
Attachment #8407602 - Attachment is obsolete: true
Attachment #8407602 - Flags: review?(bugs)
Attachment #8408349 - Flags: review?(bugs)
Attachment #8408350 - Flags: review?(bugs)
Attached patch patch 4 - IPC (obsolete) — Splinter Review
I'm going to add some tests to this patch.
Attachment #8408400 - Flags: review?(bugs)
Shouldn't this use PBackground so that you can do the IPC directly from worker threads?
In fact, you can probably do all the message brodcasting directly from the PBackground thread and never hit the main thread of the parent process.
Attached patch patch 2 - BroadcastChannel API (obsolete) — Splinter Review
Implemented using PBackground.
Attachment #8408349 - Attachment is obsolete: true
Attachment #8408350 - Attachment is obsolete: true
Attachment #8408400 - Attachment is obsolete: true
Attachment #8408349 - Flags: review?(bugs)
Attachment #8408350 - Flags: review?(bugs)
Attachment #8408400 - Flags: review?(bugs)
Attachment #8410387 - Flags: feedback?(bent.mozilla)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> Andrea, can you please send an intent to ship email to dev-platform for
> this?  Thanks!

Reminder!
Flags: needinfo?(amarchesini)
I don't think we should ship this until we have it supporting structured clones. At least not outside of certified apps.

Implementing in workers can wait since that's easy to feature test for. Though of course it'd be really awesome to have worker support too.
(In reply to comment #17)
> I don't think we should ship this until we have it supporting structured
> clones. At least not outside of certified apps.

Ah, good point!  I guess we should ship this alongside with MessageChannel stuff when that work is finished.  But we should still send out an intent to implement.

> Implementing in workers can wait since that's easy to feature test for. Though
> of course it'd be really awesome to have worker support too.

Agreed.
> > I don't think we should ship this until we have it supporting structured
> > clones. At least not outside of certified apps.

Should not change the spec first? At the moment the spec it's just about DOMStrings. No Structured clones is involved.
Flags: needinfo?(amarchesini)
I think we can count on the spec getting changed to support structured clones.
That sounds like a spec bug. Why is one postMessage() limited to strings when others can
deal with structured clones.

Andrea, want to file a spec bug?
Comment on attachment 8407600 [details] [diff] [review]
patch 1 - MessageEvent

This stuff, which is coming from the spec, is misleading.
MessageEvent.channel, but it isn't actually the channel of .ports, but just a 
string.

Could you please file a spec bug to get this changed. Perhaps
channelId
Attachment #8407600 - Flags: review?(bugs) → review-
MessageEvent.channel is going away here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25413

We should instead simply have a .name property on BroadcastChannel
Attached patch patch (obsolete) — Splinter Review
Patch updated.
Attachment #8407600 - Attachment is obsolete: true
Attachment #8410387 - Attachment is obsolete: true
Attachment #8410387 - Flags: feedback?(bent.mozilla)
Attachment #8410480 - Flags: feedback?(bent.mozilla)
In the test, please make sure that something like

x = new BroadcastChannel('foo');
y = new BroadcastChannel('foo');
x.onmessage = y.onmessage = function(e) {
  e.target.gotMessage = true;
}
x.postMessage('foo');
<wait asynchronously>
is(x.gotMessage, true);
is(x.gotMessage, undefined);

passes. I.e. calling channel.postMessage should not fire a message event on the channel object itself, but should fire it on all other broadcastchannel instances, including ones from the same window.
Attached patch bc.patch (obsolete) — Splinter Review
Jonas, I added a test for your case. It passes.
Attachment #8410480 - Attachment is obsolete: true
Attachment #8410480 - Flags: feedback?(bent.mozilla)
Attachment #8413770 - Flags: review?(bent.mozilla)
Comment on attachment 8413770 [details] [diff] [review]
bc.patch

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

::: dom/broadcastchannel/tests/test_broadcastchannel_self.html
@@ +18,5 @@
> +
> +function func(e) {
> +  is(e.target, y, "The target is !x");
> +
> +  SimpleTest.executeSoon(function() {

Just in case future code has additional asynchronous steps, maybe fire a message back at x and make sure that it only appears on x.
Attachment #8413770 - Attachment is obsolete: true
Attachment #8413770 - Flags: review?(bent.mozilla)
Attachment #8415843 - Flags: review?(bent.mozilla)
Attachment #8415845 - Flags: review?(bent.mozilla)
Attached patch patch 2 - workers (obsolete) — Splinter Review
Just a feedback here, because PBackground doesn't have worker support yet.
Attachment #8415846 - Flags: feedback?(bent.mozilla)
Comment on attachment 8415843 [details] [diff] [review]
patch 1 - BroadcastChannel API for main-thread

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

feedback on Olli just for the BroadcastChannel::Observe function. Though of course feel free to look at any other part of the patch.

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +164,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!strcmp(aTopic, "inner-window-destroyed")) {
> +    nsCOMPtr<nsISupportsPRUint64> wrapper = do_QueryInterface(aSubject);

Is this really the best way to check "your" inner window is being destroyed? Olli might have better ideas. Otherwise this is fine with me.

Another thing that I realized that we need to deal with is windows in the bfcache. For those we should probably queue all messages until the window gets displayed again, and then "flush" all queued messages.
Attachment #8415843 - Flags: feedback?(bugs)
Jonas, this patch implements postMessage(DOMString). Are you OK with this or do you want to support any? At the moment any is not possible, and in case you want to have it, we have to postpone this patch.
I'm fine with just supporting DOMString as long as we don't expose it to the web. I.e. we can expose it to only certified FirefoxOS apps, as well as on the nightly/aurora channels for desktop.

Does that sound ok?

What's needed to support 'any'?
> Does that sound ok?

Good for me.

> What's needed to support 'any'?

PBlob in PBackground.
Comment on attachment 8415843 [details] [diff] [review]
patch 1 - BroadcastChannel API for main-thread

* goes with type, not with param name.
Attachment #8415843 - Flags: feedback?(bugs) → feedback+
Attachment #8415843 - Attachment is obsolete: true
Attachment #8415843 - Flags: review?(bent.mozilla)
Attachment #8436923 - Flags: review?(bent.mozilla)
Comment on attachment 8436923 [details] [diff] [review]
patch 1 - BroadcastChannel API for main-thread

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

This lifetime management of BroadcastChannelService seems too complicated. All you really want is a refcounted hash set I think, and it should only be touched on one thread. And you definitely don't want it to be shut down via nsLayoutStatics.

What do you think of this idea:

1. Have BroadcastChannelService be a simple non-threadsafe-refcounted class with:
   a. |static BroadcastChannelService* sInstance;|
   b. |nsTHashTable<nsPtrHashKey<BroadcastChannelParent>> mAgents;|

2. BroadcastChannelService things look like this:

   BroadcastChannelService::BroadcastChannelService()
   {
     // sInstance is a raw BroadcastChannelService*.
     MOZ_ASSERT(!sInstance);
     sInstance = this;
   }

   BroadcastChannelService::~BroadcastChannelService()
   {
     MOZ_ASSERT(sInstance == this);
     sInstance = nullptr;
   }

   // static
   already_AddRefed<BroadcastChannelService>
   BroadcastChannelService::GetOrCreate()
   {
     nsRefPtr<BroadcastChannelService> instance = sInstance;
     if (!instance) {
       instance = new BroadcastChannelService();
     }
     return instance.forget();
   }

3. BroadcastChannelParent holds a member of nsRefPtr<BroadcastChannelService>.
4. BroadcastChannelParent calls BroadcastChannelService::GetOrCreate when it is constructed.
5. BroadcastChannelParent will automatically release BroadcastChannelService when it dies.

This way you don't have to worry about lifetime management and can just let the PBackground actors do it for you. The individual actors will own the service rather than the service owning the actors.

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +46,5 @@
> +
> +BroadcastChannel::BroadcastChannel(nsPIDOMWindow* aWindow,
> +                                   const nsAString& aChannel)
> +  : DOMEventTargetHelper(aWindow)
> +  , mChannel(aChannel)

I would initialize mInnerID here too, just pass it as an arg to the constructor?

@@ +49,5 @@
> +  : DOMEventTargetHelper(aWindow)
> +  , mChannel(aChannel)
> +{
> +  MOZ_ASSERT(aWindow);
> +  MOZ_COUNT_CTOR(BroadcastChannel);

Refcounted things shouldn't use manual ctor/dtor logging.

@@ +62,5 @@
> +  MOZ_ASSERT(!mActor);
> +}
> +
> +void
> +BroadcastChannel::Initialize(nsPIDOMWindow* aWindow, bool aAddObservers,

The majority of this stuff looks like it should be moved into the Constructor function so that you don't have to create a BroadcastChannel instance until you know this stuff won't fail.

@@ +70,5 @@
> +  MOZ_ASSERT(aWindow);
> +
> +  mInnerID = aWindow->WindowID();
> +
> +  nsIDocument* doc = aWindow->GetExtantDoc();

None of this will work off the main thread, so I think your 'aAddObservers' parameter is misnamed and not completely adequate :)

@@ +113,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +
> +    obs->AddObserver(this, "inner-window-destroyed", false);

Adding cycle-collected things to the observer service is generally not a good idea. I think the lifetime management of these objects needs to be considered a little more carefully. For starters, I don't think we should make this object stay alive unless there is an event listener attached. Thereafter we should be able to rely on the cycle collector to keep us alive, right?

@@ +137,5 @@
> +
> +  nsRefPtr<BroadcastChannel> bc = new BroadcastChannel(window, aChannel);
> +
> +  bc->Initialize(window, true, aRv);
> +  return bc.forget();

You should probably return null if aRv failed.

@@ +145,5 @@
> +BroadcastChannel::PostMessage(const nsAString& aMessage)
> +{
> +  if (mActor) {
> +    nsRefPtr<PostMessageRunnable> runnable =
> +      new PostMessageRunnable(mActor, aMessage);

Why do you need to use a runnable here? I think you should just call mActor->PostMessage() immediately and lose the runnable entirely.

@@ +188,5 @@
> +
> +void
> +BroadcastChannel::ActorFailed()
> +{
> +  MOZ_CRASH("Failed to create a PBackgroundChild actor!");

I'm not sure crashing is the right behavior here... But in practice it shouldn't happen so I don't know if I care too much.

@@ +228,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsIIPCBackgroundChildCreateCallback)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> +
> +NS_IMPL_ADDREF_INHERITED(BroadcastChannel, DOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(BroadcastChannel, DOMEventTargetHelper)

I think you can just use the normal NS_IMPL_ISUPPORTS_INHERITED macro here and avoid breaking things out like that.

::: dom/broadcastchannel/BroadcastChannel.h
@@ +24,5 @@
> +class BroadcastChannelChild;
> +
> +class BroadcastChannel MOZ_FINAL : public DOMEventTargetHelper
> +                                 , public nsIObserver
> +                                 , public nsIIPCBackgroundChildCreateCallback

Nit: our style guide wants this for definitions now: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

@@ +33,5 @@
> +  NS_DECL_NSIOBSERVER
> +
> +  BroadcastChannel(nsPIDOMWindow* aWindow, const nsAString& aChannel);
> +
> +  ~BroadcastChannel();

Refcounted things need private destructors.

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/BroadcastChannel.h"
> +#include "mozilla/dom/BroadcastChannelChild.h"

Always make sure the .h for this file is first.

@@ +20,5 @@
> +
> +bool
> +BroadcastChannelChild::RecvNotify(const nsString& aMessage)
> +{
> +  MOZ_ASSERT(mEventTarget);

You can't assert this, the window might have gone away and nulled out your pointer.

@@ +26,5 @@
> +  nsCOMPtr<EventTarget> eventTarget = do_QueryInterface(mEventTarget);
> +  nsRefPtr<MessageEvent> event =
> +    new MessageEvent(eventTarget, nullptr, nullptr);
> +
> +  ThreadsafeAutoSafeJSContext cx;

I believe you can get a proper JSContext from mEventTarget. You should use that rather than this thing.

@@ +27,5 @@
> +  nsRefPtr<MessageEvent> event =
> +    new MessageEvent(eventTarget, nullptr, nullptr);
> +
> +  ThreadsafeAutoSafeJSContext cx;
> +  JS::Rooted<JSString*> str(cx, JS_InternUCStringN(cx, aMessage.get(),

You don't want to intern this string! It will most likely only ever be used once. JS_NewUCStringCopyN is what you want.

@@ +42,5 @@
> +                          false /* cancelable */, value, EmptyString(),
> +                          EmptyString(), nullptr);
> +
> +  event->SetOrigin(mOrigin);
> +  event->SetTrusted(false);

This is a trusted event I believe. Maybe verify with smaug.

@@ +47,5 @@
> +
> +  bool status;
> +  mEventTarget->DispatchEvent(static_cast<Event*>(event.get()), &status);
> +
> +  return status;

Eek! Returning false here will kill the child process and the actor chain in the main process. Content JS can do that just by calling |event.preventDefault()|. You should never let that be your return value here.

::: dom/broadcastchannel/BroadcastChannelChild.h
@@ +28,5 @@
> +
> +  bool RecvNotify(const nsString& aMessage);
> +
> +private:
> +  nsRefPtr<EventTarget> mEventTarget;

Hrm, you're holding a strong ref to a cycle collected object and then not telling the cycle collector about it. That's going to be a problem...

I think you want a raw pointer (or weak ref) here? But I don't know what the lifetime is supposed to be like here, see previous comment.

::: dom/broadcastchannel/BroadcastChannelParent.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/BroadcastChannelParent.h"

Nit: These files live in the same directory, no need for the folder names. Same for other cpp files here.

::: dom/broadcastchannel/BroadcastChannelParent.h
@@ +14,5 @@
> +
> +class BroadcastChannelParent MOZ_FINAL : public PBroadcastChannelParent
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BroadcastChannelParent)

I don't see why this needs to be refcounted at all, much less in a threadsafe way.

@@ +21,5 @@
> +                         const nsAString& aChannel);
> +
> +  bool RecvPostMessage(const nsString& aMessage) MOZ_OVERRIDE;
> +
> +  void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;

Nit: please mark all your overrides ad virtual too

::: dom/broadcastchannel/BroadcastChannelService.h
@@ +14,5 @@
> +
> +class BroadcastChannelService
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BroadcastChannelService)

Anything refcounted needs at least a private destructor. Since you're using the GetOrCreate function you can also make the constructor private.

@@ +29,5 @@
> +                   const nsAString& aOrigin,
> +                   const nsAString& aChannel);
> +
> +private:
> +  nsTArray<nsRefPtr<BroadcastChannelParent>> mAgents;

I think it would be better to use a hash set here, you don't know how many of these we'll end up with and you want quick removal without walking the whole list.

::: dom/broadcastchannel/PBroadcastChannel.ipdl
@@ +4,5 @@
> +
> +include protocol PBackground;
> +
> +namespace mozilla {
> +namespace ipc {

This should be in mozilla::dom, not mozilla::ipc.

@@ +16,5 @@
> +  PostMessage(nsString message);
> +  __delete__();
> +
> +child:
> +  Notify(nsString message);

Hrm, what's to prevent the parent sending Notify at the same time the child is sending __delete__?

::: dom/broadcastchannel/moz.build
@@ +9,5 @@
> +EXPORTS.mozilla.dom += [
> +    'BroadcastChannel.h',
> +    'BroadcastChannelChild.h',
> +    'BroadcastChannelParent.h',
> +    'BroadcastChannelService.h',

Hopefully this won't need to be exported after the lifetime stuff is ironed out.

@@ +22,5 @@
> +
> +FAIL_ON_WARNINGS = True
> +
> +LOCAL_INCLUDES += [
> +    '../base',

Is this needed?

::: dom/webidl/BroadcastChannel.webidl
@@ +7,5 @@
> + * http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#broadcasting-to-other-browsing-contexts
> + */
> +
> +[Constructor(DOMString channel)]
> +interface BroadcastChannel : EventTarget {

This needs a close() function right? Otherwise these things are going to leak until the page goes away...

::: ipc/glue/BackgroundChildImpl.cpp
@@ +97,5 @@
> +// -----------------------------------------------------------------------------
> +
> +PBroadcastChannelChild*
> +BackgroundChildImpl::AllocPBroadcastChannelChild(const nsString& aOrigin,
> +                                                 const nsString& aChannel)

Can anything about these parameters be asserted?

@@ +100,5 @@
> +BackgroundChildImpl::AllocPBroadcastChannelChild(const nsString& aOrigin,
> +                                                 const nsString& aChannel)
> +{
> +  nsRefPtr<dom::BroadcastChannelChild> agent =
> +    new dom::BroadcastChannelChild(aOrigin, aChannel);

Nit: here and below I think you should do a full namespace qualification (I seem to recall that some compilers get confused on this in some cases). Or have a using statement in this file like you do in the parent impl.

@@ +111,5 @@
> +{
> +  dom::BroadcastChannelChild* child =
> +    static_cast<dom::BroadcastChannelChild*>(aActor);
> +  MOZ_ASSERT(child);
> +  NS_RELEASE(child);

Nit: I prefer assigning to a nsRefPtr with dontAddRef to be consistent with your alloc method.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +110,5 @@
>  }
>  
> +PBroadcastChannelParent*
> +BackgroundParentImpl::AllocPBroadcastChannelParent(const nsString& aOrigin,
> +                                                   const nsString& aChannel)

We need to do more than just accept the child's word for who/what its origin is. We'll need to send a principal along with this constructor message and then validate it.

Grab me on irc to see how this is supposed to work. It's new so it's still not easy... See bug 1024098.

@@ +122,5 @@
> +  return agent.forget().take();
> +}
> +
> +bool
> +BackgroundParentImpl::RecvPBroadcastChannelConstructor(

This function isn't doing anything so I just wouldn't override it at all.

@@ +146,5 @@
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(aActor);
> +
> +  BroadcastChannelParent* parent = static_cast<BroadcastChannelParent*>(aActor);
> +  MOZ_ASSERT(parent);

You already asserted aActor so this assertion is redundant.

@@ +147,5 @@
> +  MOZ_ASSERT(aActor);
> +
> +  BroadcastChannelParent* parent = static_cast<BroadcastChannelParent*>(aActor);
> +  MOZ_ASSERT(parent);
> +  NS_RELEASE(parent);

Nit: I prefer assigning to a nsRefPtr with dontAddRef to be consistent with your alloc method.
Attachment #8436923 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8415846 [details] [diff] [review]
patch 2 - workers

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

I think this should wait for bug 1013571... Then you won't have to do any main thread bouncing.
Attachment #8415846 - Flags: feedback?(bent.mozilla)
Comment on attachment 8415845 [details] [diff] [review]
patch 2 - BroadcastChannel API IPC tests

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

Will this be needed? I kinda hope we can rely on mochitest-e10s here...
> This way you don't have to worry about lifetime management and can just let
> the PBackground actors do it for you. The individual actors will own the
> service rather than the service owning the actors.

Wow. I like this approach.

> > +    obs->AddObserver(this, "inner-window-destroyed", false);
> 
> Adding cycle-collected things to the observer service is generally not a
> good idea. I think the lifetime management of these objects needs to be
> considered a little more carefully. For starters, I don't think we should
> make this object stay alive unless there is an event listener attached.
> Thereafter we should be able to rely on the cycle collector to keep us
> alive, right?

I want to talk with smaug about this.

> @@ +145,5 @@
> > +BroadcastChannel::PostMessage(const nsAString& aMessage)
> > +{
> > +  if (mActor) {
> > +    nsRefPtr<PostMessageRunnable> runnable =
> > +      new PostMessageRunnable(mActor, aMessage);
> 
> Why do you need to use a runnable here? I think you should just call
> mActor->PostMessage() immediately and lose the runnable entirely.

Yes I do. PBackground is initialized in this way:

  ipc::BackgroundChild::GetOrCreateForCurrentThread(this);

Then |BroadcastChannel::ActorCreated(ipc::PBackgroundChild* aActor)| is called and we have an actor.

but if the content code does:

var x = new BroadcastChannel('a');
var y = new BroadcastChannel('a');

x.postMessage('foobar');

x and y are created but they don't have a mActor yet. When x.ActorCreated() is called, it starts to send events, but y will not receive them because its y.ActorCreated() is not called yet. This is the reason why we have the runnable.
It doesn't have any change about nsIPrincipal yet.
Attachment #8415845 - Attachment is obsolete: true
Attachment #8436923 - Attachment is obsolete: true
Attachment #8415845 - Flags: review?(bent.mozilla)
Attachment #8439307 - Flags: review?(bent.mozilla)
I believe you can make the observer-service hold a weak reference to the observer. If nothing else you can do it by proxying through an intermediary object which holds a weak reference to the broadcast channel.

We could certainly enable collecting of broadcast channel objects that don't have any event handlers registered. However I believe that this will be an optimization that will rarely do anything useful in practice. I.e. I expect there to basically always be an event listener registered.

However the spec has added a .close() function which should "disable" the BroadcastChannel object. If that function is called the BroadcastChannel should never fire any more events and we could drop any strong references to it. (Though strictly speaking we should not unregister any event handlers when that's called).
Attachment #8439307 - Attachment description: Broadcast API → Patch 1 - Broadcast API - basic implementation
Attachment #8415846 - Attachment is obsolete: true
Attached patch patch 2 - Close method (obsolete) — Splinter Review
This second patch implements the close() method.
Attachment #8440255 - Flags: review?(bent.mozilla)
(In reply to Jonas Sicking (:sicking) from comment #43)
> I believe you can make the observer-service hold a weak reference to the
> observer. If nothing else you can do it by proxying through an intermediary
> object which holds a weak reference to the broadcast channel.

Talking with smaug, he suggested to do not use an observer for the inner-window-destroyed, but to rely on CC. So I did in the patch number 1.

The principal part is still missing.
Spec has changed again, baku says another patch will be on the way.
Attached patch patch 1 - BroadcastChannel API (obsolete) — Splinter Review
The first version of this API will allow to send just DOMString messages.
The 'any' part will be a follow up. This has been discussed with jonas.

This first patch implements:

1. webIDL
2. IPDL protocol - based on PBackground
3. main-thread and worker stuff
4. life-time of the BroadcastChannel object
5. basic mochitests

void close() method is in a separated patch.
Attachment #8439307 - Attachment is obsolete: true
Attachment #8469392 - Flags: review?(bugs)
Attached patch patch 2 - Close method (obsolete) — Splinter Review
Attachment #8440255 - Attachment is obsolete: true
Attachment #8469393 - Flags: review?(bugs)
Attached patch patch 3 - Disabled by prefs (obsolete) — Splinter Review
This API will land disabled by pref.
Attachment #8469396 - Flags: review?(bugs)
Comment on attachment 8469396 [details] [diff] [review]
patch 3 - Disabled by prefs


>+  if (key == WORKERPREF_BROADCASTCHANNEL) {
>+    sDefaultPreferences[WORKERPREF_BROADCASTCHANNEL] =
>+     Preferences::GetBool(PREF_BROADCASTCHANNEL_ENABLED, false);
2 spaces for indentation please
Attachment #8469396 - Flags: review?(bugs) → review+
Comment on attachment 8469393 [details] [diff] [review]
patch 2 - Close method

> BroadcastChannel::PostMessage(const nsAString& aMessage)
> {
>+  if (mState != StateActive) {
>+    return;
We should throw an exception here, 
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26545
http://logs.glob.uno/?c=freenode%23whatwg&s=8%20Aug%202014&e=8%20Aug%202014#c899786

>+  enum {
{ should be on its own line

Could you add some test also for the exception case.
Attachment #8469393 - Flags: review?(bugs) → review+
Comment on attachment 8469392 [details] [diff] [review]
patch 1 - BroadcastChannel API

>+  } else {
>+    // If we are in "app code", use manifest URL as unique origin since
>+    // multiple apps can share the same origin but not same broadcast messages.
>+    nsresult rv;
>+    nsCOMPtr<nsIAppsService> appsService =
>+      do_GetService("@mozilla.org/AppsService;1", &rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      aRv.Throw(rv);
>+      return;
>+    }
>+
>+    appsService->GetManifestURLByLocalId(appId, aOrigin);

Someone who understands appid vs. origin handling needs to review this.

>+class PostMessageRunnable : public nsCancelableRunnable
>+{
>+public:
>+  PostMessageRunnable(BroadcastChannelChild* aActor,
>+                      const nsAString& aMessage)
>+    : mActor(aActor)
>+    , mMessage(aMessage)
>+  {
>+  }
>+
>+  NS_IMETHODIMP Run()
>+  {
>+    mActor->SendPostMessage(mMessage);
>+    return NS_OK;
>+  }
>+
>+private:
>+  nsRefPtr<BroadcastChannelChild> mActor;
>+  nsString mMessage;
nsString? Per spec this should be structure clone


>+/* static */ already_AddRefed<BroadcastChannel>
>+BroadcastChannel::Constructor(const GlobalObject& aGlobal,
>+                              const nsAString& aChannel,
>+                              ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
>+  // Window is null in workers.
>+
>+  nsAutoString origin;
>+
>+  if (!NS_IsMainThread()) {
>+    nsRefPtr<InitializeRunnable> runnable = new InitializeRunnable(origin, aRv);
>+    runnable->Dispatch(aGlobal.Context());
>+  } else {
>+    GetOrigin(window, origin, aRv);
Why is using window right here? The spec says one should use 'incumbent settings object'.

>+BroadcastChannel::PostMessage(const nsAString& aMessage)
So we should support structure clones

>+BroadcastChannel::AddEventListener(const nsAString& aType,
>+                                   EventListener* aCallback,
>+                                   bool aCapture,
>+                                   const dom::Nullable<bool>& aWantsUntrusted,
>+                                   ErrorResult& aRv)
>+{
>+  DOMEventTargetHelper::AddEventListener(aType, aCallback, aCapture,
>+                                         aWantsUntrusted, aRv);
>+  if (!aRv.Failed()) {
>+    AddEventListenerOrEventHandler();
>+  }
>+}
Note, if the same event listener is added more that once, only the first call registers a listener.
Other calls are just no-op.
So AddEventListenerOrEventHandler looks wrong.


>+
>+void
>+BroadcastChannel::RemoveEventListener(const nsAString& aType,
>+                                      EventListener* aCallback,
>+                                      bool aCapture,
>+                                      ErrorResult& aRv)
>+{
>+  DOMEventTargetHelper::RemoveEventListener(aType, aCallback, aCapture, aRv);
>+  if (!aRv.Failed()) {
>+    RemoveEventListenerOrEventHandler();
>+  }
Note, removeEventListener doesn't fail even if you remove a non-exiting listener.
So this looks wrong.

>+void
>+BroadcastChannel::AddEventListenerOrEventHandler()
>+{
>+  if (++mEventListenersOrEventHandlersCounterCounter == 1) {
>+    AddRef();
>+  }
>+}
>+
>+void
>+BroadcastChannel::RemoveEventListenerOrEventHandler()
>+{
>+  if (--mEventListenersOrEventHandlersCounterCounter == 0) {
>+    Release();
>+  }
>+}

I think you should just call here EventListenerManager::HasListenersFor, which unfortunately is main-thread only thing atm.
So you'd need to add a threadsafe version of it, or better, just make the current version threadsafe.
EventListenerManager already knows whether it is mainthread thing, mIsMainThreadELM.
Attachment #8469392 - Flags: review?(bugs) → review-
Attached patch patch 1 - BroadcastChannel API (obsolete) — Splinter Review
Attachment #8469392 - Attachment is obsolete: true
Attachment #8470328 - Flags: review?(bugs)
Attached patch patch 2 - Close method (obsolete) — Splinter Review
Attachment #8469393 - Attachment is obsolete: true
Attached patch patch 2 - Close method (obsolete) — Splinter Review
rebased
Attachment #8470330 - Attachment is obsolete: true
Attached patch patch 3 - Disabled by prefs (obsolete) — Splinter Review
Attachment #8469396 - Attachment is obsolete: true
Comment on attachment 8470328 [details] [diff] [review]
patch 1 - BroadcastChannel API

Jonas, can you take a look at GetOrigin function and see if it matches the spec and ffos app origin check?
Attachment #8470328 - Flags: review?(jonas)
Comment on attachment 8470328 [details] [diff] [review]
patch 1 - BroadcastChannel API


>+BroadcastChannel::RemoveEventListener(const nsAString& aType,
>+                                      EventListener* aCallback,
>+                                      bool aCapture,
>+                                      ErrorResult& aRv)
>+{
>+  DOMEventTargetHelper::RemoveEventListener(aType, aCallback, aCapture, aRv);
>+
>+  if (aRv.Failed()) {
>+    return;
>+  }
>+
>+  mHasEventListeners = HasListenersFor(NS_LITERAL_STRING("message"));
>+  UpdateMustKeepAlive();
Make UpdateMustKeepAlive check whether there are event listeners


>+BroadcastChannel::UpdateMustKeepAlive()
>+{
>+  bool toKeepAlive = mHasEventListeners || mHasEventHandler;
Why you need a separate flag fro mHasEventHandler?
It is just a form of event listener

So I think we need tests for making sure having just (non-closed) broadcastchannel in worker keeps the worker alive.
And also check that broadbastchannel object in the main thread stay alive if they just have message event listeners and are not closed.
And we need tests for nested workers, and SharedWorkers


>+  bool HasListenersFor(const nsAString& aTypeWithOn)
>+  {
>+    return mListenerManager && mListenerManager->HasListenersFor(aTypeWithOn);
>+  }
s/aTypeWithOn/aType/ since this code doesn't expect "onfoo", but "foo"



> EventListenerManager::HasListenersFor(const nsAString& aEventName)
> {
>-  nsCOMPtr<nsIAtom> atom = do_GetAtom(NS_LITERAL_STRING("on") + aEventName);
>-  return HasListenersFor(atom);
>+  if (NS_IsMainThread()) {
Use mIsMainThreadELM



>+++ b/dom/events/MessageEvent.h
... 
>+  void SetOrigin(const nsAString& aOrigin)
>+  {
>+    mOrigin = aOrigin;
>+  }
I don't understand why this is needed.
Just use MessageEvent::Constructor for creating the event.


>+[Constructor(DOMString channel),
>+ Exposed=(Window,Worker)]
>+interface BroadcastChannel : EventTarget {
So given we don't implement this per spec, only support DOMString as data, 
this should certainly be exposed only in case of some permission or pref.

>+  readonly attribute DOMString name;
>+
>+  void postMessage(DOMString message);
please file a followup to support any.

>+           attribute EventHandler onmessage;
odd indentation.
Attachment #8470328 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #58)
> >+  void SetOrigin(const nsAString& aOrigin)
> >+  {
> >+    mOrigin = aOrigin;
> >+  }
> I don't understand why this is needed.
> Just use MessageEvent::Constructor for creating the event.
Ah, you may want to add the kind of ::Constructor which generated events have which takes
EventTarget* but doesn't need GlobalObject
> >+[Constructor(DOMString channel),
> >+ Exposed=(Window,Worker)]
> >+interface BroadcastChannel : EventTarget {
> So given we don't implement this per spec, only support DOMString as data, 
> this should certainly be exposed only in case of some permission or pref.

You already reviewed this patch. It's patch #3.

> >+  readonly attribute DOMString name;
> >+
> >+  void postMessage(DOMString message);
> please file a followup to support any.
> 
> >+           attribute EventHandler onmessage;
> odd indentation.

Usually we keep the WebIDL interfaces as they are in the spec. The indentation wants to keep aligned the 'attribute' keyword with/without 'readonly'.
Tests for nested workers and sharedWorkers in a separated patch.
Attachment #8470664 - Flags: review?(bugs)
Attached patch patch 1 - BroadcastChannel API (obsolete) — Splinter Review
Attachment #8470328 - Attachment is obsolete: true
Attachment #8470328 - Flags: review?(jonas)
Attachment #8470671 - Flags: review?(bugs)
Attached patch patch 2 - Close method (obsolete) — Splinter Review
Just rebased.
Attachment #8470333 - Attachment is obsolete: true
Added a test for CC/GC
Attachment #8470664 - Attachment is obsolete: true
Attachment #8470664 - Flags: review?(bugs)
Attachment #8470678 - Flags: review?(bugs)
Blocks: 1051723
(In reply to Andrea Marchesini (:baku) from comment #60)
> > >+[Constructor(DOMString channel),
> > >+ Exposed=(Window,Worker)]
> > >+interface BroadcastChannel : EventTarget {
> > So given we don't implement this per spec, only support DOMString as data, 
> > this should certainly be exposed only in case of some permission or pref.
> 
> You already reviewed this patch. It's patch #3.
I may have made mistake earlier ;)
And see also https://bugzilla.mozilla.org/show_bug.cgi?id=966439#c34
Attached patch patch 3 - Disabled by prefs (obsolete) — Splinter Review
Disabled by pref, enabled in debug builds and usable only for desktop and b2g certified apps.
Attachment #8470335 - Attachment is obsolete: true
Attachment #8470806 - Flags: review?(bugs)
Comment on attachment 8470678 [details] [diff] [review]
patch 4 - tests for workers and sharedWorkers

>       ok(true, "Worker is ready!");
>-      bc.postMessage('hello world from the window');
>+      for (var i = 0; i < 3; ++i) {
>+        SpecialPowers.forceCC();
>+        SpecialPowers.forceGC();
>+      }
>+      bc.postMessage('are you alive?');
worker stays certainly alive here because of the worker variable.
Also, you should not use worker.onmessage for the messaging here.
Just use bc.onmessage for everything.
And post the message asynchronously after forceGC/CC have been called.
Attachment #8470678 - Flags: review?(bugs) → review+
Attachment #8470678 - Attachment is obsolete: true
Comment on attachment 8470806 [details] [diff] [review]
patch 3 - Disabled by prefs

> 
>+  // GetWindowFromGlobal returns the inner window for this global, if
>+  // any, else null.
>+  static already_AddRefed<nsPIDOMWindow> GetWindowFromGlobal(JSObject* aGlobal);
This really should go to somewhere else, like to nsContentUtils.
But since you're just making this method public, fine.
(But also fine move it to nsContentUtils)


>+    nsIDocument* doc = win->GetExtantDoc();
>+    if (!doc || !doc->NodePrincipal()) {
>+      return false;
>+    }
NodePrincipal is guaranteed to return non-null.
Attachment #8470806 - Flags: review?(bugs) → review+
Comment on attachment 8470671 [details] [diff] [review]
patch 1 - BroadcastChannel API

>+BroadcastChannel::UpdateMustKeepAlive()
>+{
>+  bool toKeepAlive = HasListenersFor(NS_LITERAL_STRING("message")) ||
>+                     GetOnmessage();
What is wrong with just doing
HasListenersFor(NS_LITERAL_STRING("message"))
onfoo handlers are just event listeners in bubble phase.


But I don't see this dealing with bfcache.
We need a patch for that, and tests.
The current patch misses "a global object that is a Window object and a responsible document that is fully active" part of the spec.

(and bent needs to review PBackground related stuff)
Attachment #8470671 - Flags: review?(bugs) → review+
> But I don't see this dealing with bfcache.
> We need a patch for that, and tests.
> The current patch misses "a global object that is a Window object and a
> responsible document that is fully active" part of the spec.

This will be written in a separated patch.
Comment on attachment 8470671 [details] [diff] [review]
patch 1 - BroadcastChannel API

Bent, can you check the PBackground part? Thanks.
Attachment #8470671 - Flags: review?(bent.mozilla)
Attached patch interdiff patch 3 (obsolete) — Splinter Review
It turned out that mWorkerPrivate->GetPrincipal() doesn't work for nested workers. This patch fixes the issue.
Attachment #8472300 - Flags: review?(bugs)
Attached patch patch 3 - Disabled by prefs (obsolete) — Splinter Review
Attachment #8470806 - Attachment is obsolete: true
Comment on attachment 8472300 [details] [diff] [review]
interdiff patch 3

>+GetPrincipalFromWorkerPrivate(WorkerPrivate* aWorkerPrivate)
bent needs to review this.
(I don't know whether it is safe to iterate the parent chain without any locking)
Attachment #8472300 - Flags: review?(bugs) → review?(bent.mozilla)
Attachment #8472411 - Flags: review?(khuey)
Attached patch patch 6 - postMessage(any) (obsolete) — Splinter Review
Blobs are not supported in this patch.
Attachment #8473904 - Flags: review?(bent.mozilla)
Attachment #8472411 - Flags: review?(bugs) → review+
Attached patch patch 7 - blob (obsolete) — Splinter Review
Here the support for blobs.
Attachment #8500949 - Flags: review?(bent.mozilla)
Attached patch patch 1 - BroadcastChannel API (obsolete) — Splinter Review
I'm going to upload a new set of patches fully rebased.
Attachment #8470671 - Attachment is obsolete: true
Attachment #8470671 - Flags: review?(bent.mozilla)
Attachment #8532079 - Flags: review?(bent.mozilla)
Attached patch patch 3 - Disabled by prefs (obsolete) — Splinter Review
Attachment #8472300 - Attachment is obsolete: true
Attachment #8472302 - Attachment is obsolete: true
Attachment #8472300 - Flags: review?(bent.mozilla)
Attachment #8532081 - Flags: review?(bent.mozilla)
Attachment #8470934 - Attachment is obsolete: true
Attached patch patch 6 - postMessage(any) (obsolete) — Splinter Review
Attachment #8473904 - Attachment is obsolete: true
Attachment #8473904 - Flags: review?(bent.mozilla)
Attachment #8532083 - Flags: review?(bent.mozilla)
Attached patch patch 7 - blob (obsolete) — Splinter Review
Attachment #8500949 - Attachment is obsolete: true
Attachment #8500949 - Flags: review?(bent.mozilla)
Attachment #8532086 - Flags: review?(bent.mozilla)
Comment on attachment 8532079 [details] [diff] [review]
patch 1 - BroadcastChannel API

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

This looks really good, but there are a few big issues that need to be resolved:

1. We need security checks in the case of child process actors.
2. The WorkerFeature needs some work.
3. I don't think you need runnables to call IPDL methods.

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +5,5 @@
> +
> +#include "BroadcastChannel.h"
> +#include "BroadcastChannelChild.h"
> +#include "mozilla/dom/BroadcastChannelBinding.h"
> +#include "mozilla/dom/WorkerPrivate.h"

Hm, you added "workers" to the LOCAL_INCLUDES of the moz.build, why do you need that if you're using exported headers?

@@ +14,5 @@
> +#include "nsIAppsService.h"
> +#include "nsIDocument.h"
> +#include "nsIScriptSecurityManager.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsISupportsPrimitives.h"

Nit: alphabetize?

@@ +16,5 @@
> +#include "nsIScriptSecurityManager.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsISupportsPrimitives.h"
> +
> +using namespace mozilla::dom::workers;

Move this inside your two namespace blocks to keep it from leaking in unified builds.

@@ +24,5 @@
> +
> +namespace {
> +
> +void
> +GetOrigin(nsIPrincipal* aPrincipal, nsAString& aOrigin, ErrorResult& aRv)

MOZ_ASSERT(NS_IsMainThread())

@@ +44,5 @@
> +
> +  uint32_t appId = aPrincipal->GetAppId();
> +
> +  // If we are in "app code", use manifest URL as unique origin since
> +  // multiple apps can share the same origin but not same broadcast messages.

Hm, why not just use the principal codebase? That would have the appid+browser prefix which would do the right thing, wouldn't it? Then you wouldn't have to worry about the apps service...

@@ +75,5 @@
> +    GetOrigin(mWorkerPrivate->GetPrincipal(), mOrigin, mRv);
> +    return true;
> +  }
> +
> +private:

Private destructor please.

@@ +81,5 @@
> +  nsAString& mOrigin;
> +  ErrorResult& mRv;
> +};
> +
> +class PostMessageRunnable : public nsCancelableRunnable

It's a little weird that you don't implement Cancel here... I'd null out mActor and then check mActor in Run().

@@ +84,5 @@
> +
> +class PostMessageRunnable : public nsCancelableRunnable
> +{
> +public:
> +  PostMessageRunnable(BroadcastChannelChild* aActor,

MOZ_ASSERT(aActor)

@@ +102,5 @@
> +  nsRefPtr<BroadcastChannelChild> mActor;
> +  nsString mMessage;
> +};
> +
> +class TeardownRunnable : public nsCancelableRunnable

Same re: not implementing Cancel

@@ +105,5 @@
> +
> +class TeardownRunnable : public nsCancelableRunnable
> +{
> +public:
> +  TeardownRunnable(BroadcastChannelChild* aActor)

MOZ_ASSERT(aActor)

@@ +122,5 @@
> +};
> +
> +class BroadcastChannelFeature : public workers::WorkerFeature
> +{
> +  BroadcastChannel* mChannel;

I don't see anything that holds mChannel alive while this feature is registered, and you leave it registered for the lifetime of the worker... Obviously you can't just hold a strong ref here so I think you need to do something more clever in the Unlink()->Shutdown() call chain.

@@ +134,5 @@
> +
> +  virtual bool Notify(JSContext* aCx, workers::Status aStatus) MOZ_OVERRIDE
> +  {
> +    if (aStatus >= Canceling) {
> +      WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();

Use GetWorkerPrivateFromContext() to avoid the tls lookup

@@ +137,5 @@
> +    if (aStatus >= Canceling) {
> +      WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +      MOZ_ASSERT(workerPrivate);
> +
> +      workerPrivate->RemoveFeature(workerPrivate->GetJSContext(), this);

Just pass aCx through here

@@ +159,5 @@
> +{
> +  // Window can be null in workers
> +
> +  // Register this component to PBackground.
> +  ipc::BackgroundChild::GetOrCreateForCurrentThread(this);

This is sketchy, you're passing 'this' with a refcount of 0 to an XPCOM method that expects a stable refcount. Can we just move all this code out of the constructor and into the Constructor() function? That's much simpler than trying to do this here

@@ +161,5 @@
> +
> +  // Register this component to PBackground.
> +  ipc::BackgroundChild::GetOrCreateForCurrentThread(this);
> +
> +  if (!NS_IsMainThread()) {

If you've got an else block don't check ! here, just reverse the block order.

@@ +173,5 @@
> +    }
> +  }
> +
> +  // Register as observer for inner-window-destroyed.
> +  else {

Nit: Don't split your else like this, move the comment inside the else block.

@@ +185,5 @@
> +    }
> +  }
> +}
> +
> +BroadcastChannel::~BroadcastChannel()

This needs to somehow assert that you don't still have a feature registered

@@ +206,5 @@
> +  // Window is null in workers.
> +
> +  nsAutoString origin;
> +
> +  if (!NS_IsMainThread()) {

Reverse test and block order

@@ +207,5 @@
> +
> +  nsAutoString origin;
> +
> +  if (!NS_IsMainThread()) {
> +    nsRefPtr<InitializeRunnable> runnable = new InitializeRunnable(origin, aRv);

Hrm, workers have a PrincipalInfo now, so you might not need this any more. Or, if you do, we should fix PrincipalInfo to have the data you need here.

@@ +210,5 @@
> +  if (!NS_IsMainThread()) {
> +    nsRefPtr<InitializeRunnable> runnable = new InitializeRunnable(origin, aRv);
> +    runnable->Dispatch(aGlobal.Context());
> +  } else {
> +    nsCOMPtr<nsIGlobalObject> incumbent = mozilla::dom::GetIncumbentGlobal();

Wait, why aren't you using window here?

@@ +240,5 @@
> +void
> +BroadcastChannel::PostMessage(const nsAString& aMessage)
> +{
> +  if (mActor) {
> +    nsRefPtr<PostMessageRunnable> runnable =

Wait, why don;t you just call the actor message directly? Why bounce through the event loop?

@@ +281,5 @@
> +  mPendingMessages.Clear();
> +}
> +
> +void
> +BroadcastChannel::Shutdown()

I think you should remove the feature here...

@@ +294,5 @@
> +  if (mActor) {
> +    mActor->SetEventTarget(nullptr);
> +
> +    nsRefPtr<TeardownRunnable> runnable = new TeardownRunnable(mActor);
> +    NS_DispatchToCurrentThread(runnable);

Why do you need a runnable?

@@ +356,5 @@
> +
> +void
> +BroadcastChannel::UpdateMustKeepAlive()
> +{
> +  bool toKeepAlive = HasListenersFor(NS_LITERAL_STRING("message"));

It seems wasteful to call this... You know if you're being called from AddEventListener/RemoveEventListener/SetOnmessage, why enumerate?

@@ +376,5 @@
> +                          const char16_t* aData)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (strcmp(aTopic, "inner-window-destroyed")) {

I'd just MOZ_ASSERT(!strcmp(...))

@@ +381,5 @@
> +    return NS_OK;
> +  }
> +
> +  // If the window id destroyed we have to release the reference that we are
> +  // keeping.

This comment doesn't make sense to me...

::: dom/broadcastchannel/BroadcastChannel.h
@@ +8,5 @@
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "nsIIPCBackgroundChildCreateCallback.h"
> +#include "nsIObserver.h"

You need nsTArray and nsRefPtr headers.

@@ +24,5 @@
> +  , public nsIObserver
> +{
> +public:
> +  NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK
> +  NS_DECL_NSIOBSERVER

These two can be private

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +6,5 @@
> +#include "BroadcastChannelChild.h"
> +#include "BroadcastChannel.h"
> +#include "mozilla/dom/MessageEvent.h"
> +#include "mozilla/dom/MessageEventBinding.h"
> +#include "mozilla/ipc/PBackgroundChild.h"

You need some kind of jsapi include here

@@ +21,5 @@
> +
> +bool
> +BroadcastChannelChild::RecvNotify(const nsString& aMessage)
> +{
> +  nsCOMPtr<EventTarget> eventTarget = do_QueryInterface(mEventTarget);

You don't need QI here...

@@ +27,5 @@
> +  if (!eventTarget) {
> +    return true;
> +  }
> +
> +  ThreadsafeAutoSafeJSContext cx;

This isn't right any more. Now you need to do:

  DOMEventTargetHelper* deth = DOMEventTargetHelper::FromSupports(eventTarget);
  MOZ_ASSERT(deth);

  AutoJSAPI autoJS;
  if (!autoJS.Init(deth->GetParentObject())) {
    NS_WARNING("Dropping message");
    return true;
  }

  JS::Rooted<JSString*> str(autoJS.cx(), ...);
  ...

@@ +31,5 @@
> +  ThreadsafeAutoSafeJSContext cx;
> +  JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx, aMessage.get(),
> +                                                    aMessage.Length()));
> +  if (!str) {
> +    JS_ClearPendingException(cx);

I think this is not necessary, JS_NewUCStringCopyN shouldn't set an exception ever. If it fails it's because we're out of memory and there won't be an exception set on purpose.

@@ +32,5 @@
> +  JS::Rooted<JSString*> str(cx, JS_NewUCStringCopyN(cx, aMessage.get(),
> +                                                    aMessage.Length()));
> +  if (!str) {
> +    JS_ClearPendingException(cx);
> +    return false;

This isn't what you want, this will kill the process or channel. I'd just warn and return true.

@@ +48,5 @@
> +  nsRefPtr<MessageEvent> event =
> +    MessageEvent::Constructor(mEventTarget, NS_LITERAL_STRING("message"),
> +                              init, rv);
> +  if (rv.Failed()) {
> +    return false;

Warn and return true here too.

::: dom/broadcastchannel/BroadcastChannelChild.h
@@ +15,5 @@
> +
> +class BroadcastChannelChild MOZ_FINAL : public PBroadcastChannelChild
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BroadcastChannelChild)

Why threadsafe?

@@ +18,5 @@
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BroadcastChannelChild)
> +
> +  BroadcastChannelChild(const nsAString& aOrigin,
> +                        const nsAString& aChannel);

If you make BackgroundChildImpl a friend then you can move this to the private block.

@@ +25,5 @@
> +  {
> +    mEventTarget = aEventTarget;
> +  }
> +
> +  bool RecvNotify(const nsString& aMessage);

virtual/MOZ_OVERRIDE

@@ +28,5 @@
> +
> +  bool RecvNotify(const nsString& aMessage);
> +
> +private:
> +  ~BroadcastChannelChild() {

I'd move this to the cpp

@@ +33,5 @@
> +    MOZ_ASSERT(!mEventTarget);
> +  }
> +
> +  // This raw pointer is the parent and it's kept alive until this object is
> +  // not fully deleted.

This comment doesn't make sense ("not"?)

::: dom/broadcastchannel/BroadcastChannelParent.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +BroadcastChannelParent::BroadcastChannelParent(const nsAString& aOrigin,

All these methods should AssertIsOnBackgroundThread()

@@ +25,5 @@
> +
> +bool
> +BroadcastChannelParent::RecvPostMessage(const nsString& aMessage)
> +{
> +  MOZ_ASSERT(mService, "PostMessage is called after a shutdown!");

This can't be asserted, a hacked child could send this out of order. You just need to handle it (return false).

@@ +33,5 @@
> +
> +bool
> +BroadcastChannelParent::RecvShutdown()
> +{
> +  MOZ_ASSERT(mService, "Shutdown has been already called!");

Here too.

::: dom/broadcastchannel/BroadcastChannelParent.h
@@ +16,5 @@
> +{
> +public:
> +  BroadcastChannelParent(const nsAString& aOrigin,
> +                         const nsAString& aChannel);
> +  ~BroadcastChannelParent();

If you make BackgroundParentImpl a friend then you can hide these in private blocks.

::: dom/broadcastchannel/BroadcastChannelService.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "BroadcastChannelService.h"
> +#include "mozilla/StaticPtr.h"

Looks unused.

@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static BroadcastChannelService* sInstance = nullptr;

This needs to be in an anonymous namespace.

@@ +10,5 @@
> +namespace dom {
> +
> +static BroadcastChannelService* sInstance = nullptr;
> +
> +BroadcastChannelService::BroadcastChannelService()

All these methods should AssertIsOnBackgroundThread()

@@ +17,5 @@
> +  MOZ_ASSERT(!sInstance);
> +  sInstance = this;
> +}
> +
> +BroadcastChannelService::~BroadcastChannelService()

Also MOZ_ASSERT(mAgents.IsEmpty())

@@ +35,5 @@
> +  return instance.forget();
> +}
> +
> +void
> +BroadcastChannelService::RegisterActor(BroadcastChannelParent* aParent)

MOZ_ASSERT(aParent)

@@ +42,5 @@
> +  mAgents.PutEntry(aParent);
> +}
> +
> +void
> +BroadcastChannelService::UnregisterActor(BroadcastChannelParent* aParent)

MOZ_ASSERT(aParent)

@@ +50,5 @@
> +}
> +
> +namespace {
> +
> +struct PostMessageData

Nit: Mark as MOZ_STACK_CLASS and MOZ_FINAL

@@ +60,5 @@
> +    : mParent(aParent)
> +    , mMessage(aMessage)
> +    , mOrigin(aOrigin)
> +    , mChannel(aChannel)
> +  {}

MOZ_ASSERT stuff? And MOZ_COUNT_CTOR/MOZ_COUNT_DTOR

@@ +65,5 @@
> +
> +  BroadcastChannelParent* mParent;
> +  const nsAString& mMessage;
> +  const nsAString& mOrigin;
> +  const nsAString& mChannel;

Make these concrete strings, then make CheckAndDeliver() take concrete refs too. That way you won't construct a ton of temporaries as you iterate the hashset.

@@ +71,5 @@
> +
> +PLDHashOperator
> +PostMessageEnumerator(nsPtrHashKey<BroadcastChannelParent>* aKey, void* aPtr)
> +{
> +  PostMessageData *data = static_cast<PostMessageData*>(aPtr);

Nit: Use |auto* data| to avoid typing the type twice.

@@ +72,5 @@
> +PLDHashOperator
> +PostMessageEnumerator(nsPtrHashKey<BroadcastChannelParent>* aKey, void* aPtr)
> +{
> +  PostMessageData *data = static_cast<PostMessageData*>(aPtr);
> +  BroadcastChannelParent* parent = aKey->GetKey();

MOZ_ASSERT(parent);

@@ +84,5 @@
> +
> +} // anonymous namespace
> +
> +void
> +BroadcastChannelService::PostMessage(BroadcastChannelParent* aParent,

MOZ_ASSERT(aParent)

::: dom/broadcastchannel/PBroadcastChannel.ipdl
@@ +13,5 @@
> +  manager PBackground;
> +
> +parent:
> +  PostMessage(nsString message);
> +  Shutdown();

Maybe call this Close to mirror the API?

::: dom/broadcastchannel/moz.build
@@ +9,5 @@
> +EXPORTS.mozilla.dom += [
> +    'BroadcastChannel.h',
> +    'BroadcastChannelChild.h',
> +    'BroadcastChannelParent.h',
> +    'BroadcastChannelService.h',

Why does this need to be exported?

::: dom/broadcastchannel/tests/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +MOCHITEST_MANIFESTS += ['mochitest.ini']

Can you just move this to the parent dir's moz.build file? Then you wouldn't need this file. E.g.:

  dom/broadcastchannel/moz.build:

  + MOCHITEST_MANIFESTS += ['tests/mochitest.ini']

::: dom/events/EventListenerManager.cpp
@@ +1241,5 @@
> +  uint32_t count = mListeners.Length();
> +  for (uint32_t i = 0; i < count; ++i) {
> +    Listener* listener = &mListeners.ElementAt(i);
> +    if (listener->mTypeString == aEventName) {
> +      return true;

Why not just:

  if (mListeners[i].mTypeString == aEventName) {
    return true;
  }

::: ipc/glue/BackgroundChildImpl.cpp
@@ +6,5 @@
>  
>  #include "FileDescriptorSetChild.h"
>  #include "mozilla/Assertions.h"
> +#include "mozilla/dom/BroadcastChannelChild.h"
> +#include "mozilla/dom/PBroadcastChannelChild.h"

You shouldn't need this one.

@@ +195,5 @@
> +// -----------------------------------------------------------------------------
> +
> +dom::PBroadcastChannelChild*
> +BackgroundChildImpl::AllocPBroadcastChannelChild(const nsString& aOrigin,
> +                                                 const nsString& aChannel)

These arguments need to be verified... Specifically, you probably want to do some checks on the main thread for the origin if this is a child process actor. And this probably needs a PrincipalInfo argument that can be verified against the ContentParent just like we are doing for ServiceWorker registrations.

::: ipc/glue/BackgroundChildImpl.h
@@ +74,5 @@
>    DeallocPFileDescriptorSetChild(PFileDescriptorSetChild* aActor) MOZ_OVERRIDE;
> +
> +  virtual PBroadcastChannelChild*
> +  AllocPBroadcastChannelChild(const nsString& aOrigin,
> +                              const nsString& aChannelg) MOZ_OVERRIDE;

Typo: "aChannelg"

::: ipc/glue/BackgroundParentImpl.cpp
@@ +23,5 @@
>  #endif
>  
>  using mozilla::ipc::AssertIsOnBackgroundThread;
>  
> +using namespace mozilla::dom;

I'd rather you not do this, unified builds make this a footgun. Just put this in the functions you need it in.
Attachment #8532079 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8532081 [details] [diff] [review]
patch 3 - Disabled by prefs

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

::: b2g/app/b2g.js
@@ +1055,5 @@
>  #endif
>  
> +// BroadcastChannel API
> +#ifdef RELEASE_BUILD
> +pref("dom.broadcastChannel.enabled", false);

Hrm, won't this mean that gaia apps suddenly stop working when we get to Release?

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +66,5 @@
> +  if (principal) {
> +    return principal;
> +  }
> +
> +  // Walk up to our containing page

You shouldn't ever need to do this, the worker should always have a principal

@@ +263,5 @@
> +  workerPrivate->AssertIsOnWorkerThread();
> +
> +  nsRefPtr<CheckPermissionRunnable> runnable =
> +    new CheckPermissionRunnable(workerPrivate);
> +  runnable->Dispatch(aCx);

This can (theoretically) fail, in which case you'd need to clear any exceptions.

::: dom/broadcastchannel/BroadcastChannel.h
@@ +30,5 @@
>  
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BroadcastChannel,
>                                             DOMEventTargetHelper)
>  
> +  static bool Enabled(JSContext* aCx, JSObject* aGlobal);

I'd call this "IsEnabled"
Attachment #8532081 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8532083 [details] [diff] [review]
patch 6 - postMessage(any)

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

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +24,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +class BroadcastChannelMessage MOZ_FINAL

Hopefully if you get rid of the extra runnables you won't need this class any more.

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +13,3 @@
>  #include "mozilla/ipc/PBackgroundChild.h"
>  
> +using namespace mozilla::dom::workers;

Nit: Move this into a block somehow (maybe in RecvNotify)

::: dom/broadcastchannel/PBroadcastChannel.ipdl
@@ +9,4 @@
>  namespace mozilla {
>  namespace dom {
>  
> +struct BroadcastChannelMessageData

Why is this struct needed? How about just passing SerializedStructuredCloneBuffer directly?
Attachment #8532083 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8532086 [details] [diff] [review]
patch 7 - blob

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

r- for the StructuredCloneUtils problems...

::: dom/ipc/Blob.cpp
@@ +2637,5 @@
>      mEventTarget = do_GetCurrentThread();
>      MOZ_ASSERT(mEventTarget);
>    }
> +
> +  CommonInit(aParams);

I think I did this in another patch already ;)

::: dom/ipc/StructuredCloneUtils.cpp
@@ -25,5 @@
>  
>  void
>  Error(JSContext* aCx, uint32_t aErrorId)
>  {
> -  MOZ_ASSERT(NS_IsMainThread());

You can't remove these... NS_DOMStructuredCloneError can't be called off the main thread.
Comment on attachment 8532086 [details] [diff] [review]
patch 7 - blob

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

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +177,5 @@
> +
> +    const nsTArray<nsRefPtr<File>>& blobs = mData->mClosure.mBlobs;
> +    for (uint32_t i = 0, len = blobs.Length(); i < len; ++i) {
> +      PBlobChild* blobChild =
> +        BackgroundChild::GetOrCreateActorForBlob(backgroundManager, blobs[i]);

MOZ_ASSERT(blobChild)

::: dom/broadcastchannel/BroadcastChannelParent.cpp
@@ +74,5 @@
> +    return;
> +  }
> +
> +  // Duplicate the data for this parent.
> +  BroadcastChannelMessageData newData(aData);

You don't need to duplicate all the data, just the blobs...
Attachment #8532086 - Flags: review?(bent.mozilla) → review-
Thanks for reviewing these patches!

> > +  uint32_t appId = aPrincipal->GetAppId();
> > +
> > +  // If we are in "app code", use manifest URL as unique origin since
> > +  // multiple apps can share the same origin but not same broadcast messages.
> 
> Hm, why not just use the principal codebase? That would have the
> appid+browser prefix which would do the right thing, wouldn't it? Then you
> wouldn't have to worry about the apps service...

do you mean nsIPrincipal::URI ? I don't think it contains the appId too.

> > +class BroadcastChannelFeature : public workers::WorkerFeature
> > +{
> > +  BroadcastChannel* mChannel;
> 
> I don't see anything that holds mChannel alive while this feature i
> registered, and you leave it registered for the lifetime of the worker...
> Obviously you can't just hold a strong ref here so I think you need to do
> something more clever in the Unlink()->Shutdown() call chain.

What about if I use cc? I mean moving the RemoveFeature() in Shutdown() should be enough.
Correct?

> @@ +207,5 @@
> > +
> > +  nsAutoString origin;
> > +
> > +  if (!NS_IsMainThread()) {
> > +    nsRefPtr<InitializeRunnable> runnable = new InitializeRunnable(origin, aRv);
> 
> Hrm, workers have a PrincipalInfo now, so you might not need this any more.
> Or, if you do, we should fix PrincipalInfo to have the data you need here.

Yep, but I still have to create the nsIPrincipal and from there to get the origin.

> > +    nsCOMPtr<nsIGlobalObject> incumbent = mozilla::dom::GetIncumbentGlobal();
> 
> Wait, why aren't you using window here?

The spec says we have to use the incumbentGlobal that can be different than the window global.

> > +  if (mActor) {
> > +    nsRefPtr<PostMessageRunnable> runnable =
> 
> Wait, why don;t you just call the actor message directly? Why bounce through
> the event loop?

Think about this scenario:

var a = new BroadcastChannel('foo');
var b = new BroadcastChannel('foo');
b.onmessage = something;
a.postMessage('1');

I need to use the event loop because otherwise, when a receives actorCreated(), the first message will be sent, but b didn't have the actor yet and it will not receive the message. There is a similar race condition about ordering.
I wrote a mochitest for it.


> > +  if (mActor) {
> > +    mActor->SetEventTarget(nullptr);
> > +
> > +    nsRefPtr<TeardownRunnable> runnable = new TeardownRunnable(mActor);
> > +    NS_DispatchToCurrentThread(runnable);
> 
> Why do you need a runnable?

Because if I call the message from the UNLINK() we have a failure.

> @@ +356,5 @@
> > +
> > +void
> > +BroadcastChannel::UpdateMustKeepAlive()
> > +{
> > +  bool toKeepAlive = HasListenersFor(NS_LITERAL_STRING("message"));
> 
> It seems wasteful to call this... You know if you're being called from
> AddEventListener/RemoveEventListener/SetOnmessage, why enumerate?

Comment 52 from smaug. Otherwise you have to re-implement the logic already available in HasListenersFor.

>   if (mListeners[i].mTypeString == aEventName) {
>     return true;
>   }

no match for ‘operator[]’ in ‘((mozilla::EventListenerManager*)this)->mozilla::EventListenerManager::mListeners[i]’

mListeners is a nsAutoTObserverArray.
Attached patch patch 1 - interdiff (obsolete) — Splinter Review
Here the interdiff
Attachment #8540103 - Flags: review?(bent.mozilla)
The whole patch.
Attachment #8532079 - Attachment is obsolete: true
Attachment #8540104 - Flags: review?(bent.mozilla)
Attachment #8532081 - Attachment is obsolete: true
Attachment #8540104 - Attachment description: bc.patch → Patch 1 - BroadcatsChannel API
rebased
Attachment #8472411 - Attachment is obsolete: true
rebased
Attachment #8470673 - Attachment is obsolete: true
> > +  // Walk up to our containing page
> 
> You shouldn't ever need to do this, the worker should always have a principal

Not in sub-workers. I changed that with something better but still I must go up to the parent worker.
Attachment #8532083 - Attachment is obsolete: true
Attached patch patch 7 - blob (obsolete) — Splinter Review
Attachment #8532086 - Attachment is obsolete: true
Attachment #8540109 - Flags: review?(bent.mozilla)
Comment on attachment 8540109 [details] [diff] [review]
patch 7 - blob

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

r- for StructuredCloneUtils.cpp changes.

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +193,5 @@
> +    PBackgroundChild* backgroundManager = mActor->Manager();
> +    MOZ_ASSERT(backgroundManager);
> +
> +    const nsTArray<nsRefPtr<File>>& blobs = mData->mClosure.mBlobs;
> +    for (uint32_t i = 0, len = blobs.Length(); i < len; ++i) {

Before you do this do:

  if (!blobs.IsEmpty()) {
    message.blobsChild().SetCapacity(blobs.Length());
  }

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +76,5 @@
>    StructuredCloneData cloneData;
>    cloneData.mData = buffer.data;
>    cloneData.mDataLength = buffer.dataLength;
>  
> +  for (uint32_t i = 0, len = aData.blobsChild().Length(); i < len; ++i) {

Similar here:

  if (!aData.blobsChild().IsEmpty()) {
    cloneData.mClosure.mBlobs.SetCapacity(aData.blobsChild().Length());
  }

::: dom/broadcastchannel/BroadcastChannelParent.cpp
@@ +98,5 @@
> +      nsRefPtr<FileImpl> impl =
> +        static_cast<BlobParent*>(newData.blobsParent()[i])->GetBlobImpl();
> +
> +      PBlobParent* blobParent =
> +        BackgroundParent::GetOrCreateActorForBlobImpl(Manager(), impl);

This can fail if the child crashes, you should probably stop trying to make more blobs and not send the Notify message.

::: dom/broadcastchannel/PBroadcastChannel.ipdl
@@ +9,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
>  struct BroadcastChannelMessageData

Now this struct is identical to ClonedMessageData from dom/ipc/DOMTypes.ipdlh. Let's just keep the one.

::: dom/ipc/StructuredCloneUtils.cpp
@@ -25,5 @@
>  
>  void
>  Error(JSContext* aCx, uint32_t aErrorId)
>  {
> -  MOZ_ASSERT(NS_IsMainThread());

See previous comment, this can't be removed.
Attachment #8540109 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8540103 [details] [diff] [review]
patch 1 - interdiff

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

I think this is pretty much done. Please post changes in an interdiff and let me stamp it again and I think we're finished!

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +77,5 @@
>  
>    bool MainThreadRun() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();

Need to ensure that you don't have a NullPrincipal here. Those kinds of principals can't talk to any other principal.

@@ +95,5 @@
>  
>  private:
>    WorkerPrivate* mWorkerPrivate;
>    nsAString& mOrigin;
> +  PrincipalInfo* mPrincipalInfo;

Shouldn't this be PrincipalInfo& to match the others? You don't allow a null mPrincipalInfo anyway.

@@ +100,4 @@
>    ErrorResult& mRv;
>  };
>  
>  class PostMessageRunnable : public nsCancelableRunnable

I don't think you should inherit nsCancelableRunnable because that has threadsafe refcounting, and this class is entirely bound to a single thread. If this was actually threadsafe then this code wouldn't work really. And by using your own non-threadsafe refcount you get thread checking for free.

I filed bug 1119956 to clean up nsCancelableRunnable a bit.

You also need MOZ_FINAL here.

@@ +102,5 @@
>  
>  class PostMessageRunnable : public nsCancelableRunnable
>  {
>  public:
>    PostMessageRunnable(BroadcastChannelChild* aActor,

Needs a private destructor.

@@ +117,1 @@
>      mActor->SendPostMessage(mMessage);

If this runnable is canceled then mActor will be null...

But since you're using an actor in a runnable on the event loop it's possible that between the time you dispatched and the time you run that the actor has been destroyed. You won't touch a bad pointer because you're using refcounting, but any Send() calls will abort. You might want to add an ActorDestroy override to BroadcastChannelChild and have some way of checking before sending here.

Same for SendClose() below.

@@ +128,5 @@
>    nsRefPtr<BroadcastChannelChild> mActor;
>    nsString mMessage;
>  };
>  
>  class TeardownRunnable : public nsCancelableRunnable

This shouldn't use nsCancelableRunnable either, and should have MOZ_FINAL.

@@ +131,5 @@
>  
>  class TeardownRunnable : public nsCancelableRunnable
>  {
>  public:
>    TeardownRunnable(BroadcastChannelChild* aActor)

Needs a private destructor.

@@ +140,5 @@
>  
>    NS_IMETHODIMP Run()
>    {
> +    MOZ_ASSERT(mActor);
> +    mActor->SendClose();

mActor could be null...

@@ +234,5 @@
>      }
>  
>      GetOrigin(principal, origin, aRv);
> +
> +    aRv = PrincipalToPrincipalInfo(principal, &principalInfo);

This needs to watch for a NullPrincipal too.

@@ +241,5 @@
> +    }
> +
> +  } else {
> +    JSContext* cx = aGlobal.Context();
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);

Let's make |workerPrivate| a variable outside the |if (NS_IsMainThread()) { }| block, set to nullptr. Then you set to a real value here. Then below you won't have to check the thread or get the worker out of tls again, you can simply check if |workerPrivate| is null or not to know what branch to take.

@@ +246,5 @@
> +    MOZ_ASSERT(workerPrivate);
> +
> +    nsRefPtr<InitializeRunnable> runnable =
> +      new InitializeRunnable(workerPrivate, origin, &principalInfo, aRv);
> +    runnable->Dispatch(aGlobal.Context());

Pass |cx| here

@@ +272,5 @@
> +      obs->AddObserver(bc, "inner-window-destroyed", false);
> +    }
> +  }
> +
> +  else {

Nit: cuddle else

@@ +279,5 @@
> +
> +    bc->mWorkerFeature = new BroadcastChannelFeature(bc);
> +    JSContext* cx = workerPrivate->GetJSContext();
> +    if (NS_WARN_IF(!workerPrivate->AddFeature(cx, bc->mWorkerFeature))) {
> +      NS_WARNING("Failed to register the BroadcastChannel worker feature.");

You should return null here since there's no way to make this safe without keeping the worker alive.

::: dom/broadcastchannel/BroadcastChannel.h
@@ +72,5 @@
>    void Shutdown();
>  
>  private:
> +  BroadcastChannel(nsPIDOMWindow* aWindow,
> +                   const mozilla::ipc::PrincipalInfo& aPrincipalInfo,

Nit: Use a class-private typedef for this so you don't have to add mozilla::ipc:: in more than one place.

::: dom/broadcastchannel/BroadcastChannelParent.cpp
@@ +9,3 @@
>  #include "mozilla/unused.h"
>  
> +using namespace mozilla::ipc;

Nit: Move inside the next block

@@ +34,5 @@
>  {
> +  AssertIsOnBackgroundThread();
> +
> +  if (!mService) {
> +    return false;

Nit: You could add a warning here

@@ +47,5 @@
>  {
> +  AssertIsOnBackgroundThread();
> +
> +  if (!mService) {
> +    return false;

And here

@@ +67,2 @@
>    if (mService) {
>      mService->UnregisterActor(this);

Nit: You could comment here that 'this' is about to be deleted and that will release mService.

@@ +76,5 @@
>  {
> +  AssertIsOnBackgroundThread();
> +
> +  if (aOrigin == mOrigin && aChannel == mChannel) {
> +    unused << SendNotify(nsString(aMessage));

No need for this temporary now

::: dom/broadcastchannel/BroadcastChannelParent.h
@@ +25,5 @@
> +  virtual bool RecvPostMessage(const nsString& aMessage) MOZ_OVERRIDE;
> +
> +  virtual bool RecvClose() MOZ_OVERRIDE;
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;

These IPDL methods can be private, they should only ever be called from IPDL via the base class actor.

::: dom/broadcastchannel/BroadcastChannelService.h
@@ +4,5 @@
>  
>  #ifndef mozilla_dom_BroadcastChannelService_h
>  #define mozilla_dom_BroadcastChannelService_h
>  
> +#include "BroadcastChannelParent.h"

Just forward-declare this instead.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +25,5 @@
>  #else
>  #define ASSERT_UNLESS_FUZZING(...) MOZ_ASSERT(false)
>  #endif
>  
> +using mozilla::dom::ContentParent;

Nit: Move inside the mozilla::ipc block below. You could also add |using mozilla::dom::BroadcastChannelParent;| to avoid having to add that everywhere.

@@ +226,5 @@
> +}
> +
> +namespace {
> +
> +class CheckPrincipalRunnable : MOZ_FINAL public nsRunnable

MOZ_FINAL should come before the :

@@ +249,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(mPrincipalInfo);
> +    AssertAppPrincipal(mContentParent, principal);

I think AssertAppPrincipal will do the right thing for NullPrincipal, but please double check?

@@ +291,5 @@
> +  nsRefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
> +
> +  // If the ContentParent is null we are dealing with a same-process actor.
> +  if (!parent) {
> +    return true;

Please assert here that |aPrincipalInfo| is not a NullPrincipal

@@ +297,5 @@
> +
> +  nsRefPtr<CheckPrincipalRunnable> runnable =
> +    new CheckPrincipalRunnable(parent.forget(), aPrincipalInfo, aOrigin);
> +  nsresult rv = NS_DispatchToMainThread(runnable);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));

If you do it this way you need to make |rv| be DebugOnly<nsresult>

::: ipc/glue/BackgroundParentImpl.h
@@ +65,5 @@
>                                 const nsString& aChannel) MOZ_OVERRIDE;
>  
>    virtual bool
> +  RecvPBroadcastChannelConstructor(PBroadcastChannelParent* actor,
> +                                   const PrincipalInfo& pInfo,

Nit: Call this 'aPrincipalInfo' like above?
Attachment #8540103 - Flags: review?(bent.mozilla) → review-
Attached patch interdiffSplinter Review
Attachment #8540103 - Attachment is obsolete: true
Attachment #8547023 - Flags: review?(bent.mozilla)
Attached patch patch 7 - blobSplinter Review
Attachment #8540109 - Attachment is obsolete: true
Attachment #8547026 - Flags: review?(bent.mozilla)
Comment on attachment 8547026 [details] [diff] [review]
patch 7 - blob

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

Looks good!

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +206,3 @@
>      }
> +
> +    for (uint32_t i = 0, len = blobs.Length(); i < len; ++i) {

Nit: Move this inside the |if (!IsEmpty())| block above.

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +81,5 @@
> +  if (!aData.blobsChild().IsEmpty()) {
> +    cloneData.mClosure.mBlobs.SetCapacity(aData.blobsChild().Length());
> +  }
> +
> +  for (uint32_t i = 0, len = aData.blobsChild().Length(); i < len; ++i) {

Nit: Move this inside the |if (!IsEmpty())| block above.
Attachment #8547026 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8547023 [details] [diff] [review]
interdiff

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

This looks great!

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +244,5 @@
>    // Window is null in workers.
>  
>    nsAutoString origin;
>    PrincipalInfo principalInfo;
> +  WorkerPrivate* workerPrivate = nullptr;

I think you're still checking |NS_IsMainThread| twice here right? Instead on the second time you can just check if |workerPrivate| is null or not to save the TLS hit.
Attachment #8547023 - Flags: review?(bent.mozilla) → review+
Attachment #8547714 - Flags: review?(bent.mozilla)
Comment on attachment 8547714 [details] [diff] [review]
patch 8 - race issue

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

These changes look fine but we need another patch to fix this issue:

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +39,4 @@
>  }
>  
>  bool
>  BroadcastChannelChild::RecvNotify(const ClonedMessageData& aData)

Hrm, actually, I missed this before. I think you need to clean any blobs up here... If you take the early return below then you'll leak them.

You need to turn all the PBlobChilds into nsRefPtr<File> before you return.
Attachment #8547714 - Flags: review?(bent.mozilla) → review+
Attachment #8547837 - Flags: review?(bent.mozilla)
Comment on attachment 8547837 [details] [diff] [review]
patch 9 - leak of blobs

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

Thanks!

Are there any other places where we should handle this pattern?

::: dom/broadcastchannel/BroadcastChannelChild.cpp
@@ +40,5 @@
>  
>  bool
>  BroadcastChannelChild::RecvNotify(const ClonedMessageData& aData)
>  {
> +  // Let's create an array of Files in order to do not leak them.

I'd just say something like "Make sure to retrieve all blobs from the message before returning to avoid leaking their actors."

@@ +91,5 @@
>    const SerializedStructuredCloneBuffer& buffer = aData.data();
>    StructuredCloneData cloneData;
>    cloneData.mData = buffer.data;
>    cloneData.mDataLength = buffer.dataLength;
> +  cloneData.mClosure.mBlobs.AppendElements(files);

Let's use SwapElements here.
Attachment #8547837 - Flags: review?(bent.mozilla) → review+
Release Note Request (optional, but appreciated)
[Why is this notable]: This is a HTML5 API. Nice and easy to use. It's a valid alternative to simple-uses of SharedWorkers.
[Suggested wording]: BroadcastChannel API implemented
[Links (documentation, blog post, etc)]: http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#broadcastchannel-settings-object - plus, we are planning to write a blog post on hacks.mozilla.org
relnote-firefox: --- → ?
Yay! This is very exciting!

It would be good to mention in the blog post that it's possible to write a polyfill for this API by using localStorage and its change notifications. Hopefully someone out there will step up and do that so that we can link to them.
Reverted for causing test failures across the board.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec13e4c1e1d6

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5447347&repo=mozilla-inbound (waiting on retriggers to confirm, but strongly looking that way)
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5442027&repo=mozilla-inbound

And a pile of various B2G failures.

Please verify that this is fully green on Try before re-landing.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #114)
> html#?job_id=5447347&repo=mozilla-inbound (waiting on retriggers to confirm,
> but strongly looking that way)

Confirmed.
Have added this to Firefox 38 Dev Edition release notes as: 	

BroadcastChannel API implemented (more at hacks.mozilla.org)

We need a proper hacks.mozilla.org link to replace the current placeholder.
Depends on: 1140775
I updated the release note for 38 with a link to https://hacks.mozilla.org/2015/02/broadcastchannel-api-in-firefox-38/.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.