1274363 - BroadcastChannel dispatches event in wrong order

Status: RESOLVED FIXED

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

Description

[Marijn]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.36 Safari/537.36

Steps to reproduce:

Run the following testharness.js based test:

async_test(t => {
    let c1 = new BroadcastChannel('order');
    let c2 = new BroadcastChannel('order');
    let c3 = new BroadcastChannel('order');

    let events = [];
    let doneCount = 0;
    let handler = t.step_func(e => {
      events.push(e);
      if (e.data == 'done') {
        doneCount++;
        if (doneCount == 2) {
          assert_equals(events.length, 6);
          assert_equals(events[0].target, c2, 'target for event 0');
          assert_equals(events[0].data, 'from c1');
          assert_equals(events[1].target, c3, 'target for event 1');
          assert_equals(events[1].data, 'from c1');
          assert_equals(events[2].target, c1, 'target for event 2');
          assert_equals(events[2].data, 'from c3');
          assert_equals(events[3].target, c2, 'target for event 3');
          assert_equals(events[3].data, 'from c3');
          assert_equals(events[4].target, c1, 'target for event 4');
          assert_equals(events[4].data, 'done');
          assert_equals(events[5].target, c3, 'target for event 5');
          assert_equals(events[5].data, 'done');
          t.done();
        }
      }
    });
    c1.onmessage = handler;
    c2.onmessage = handler;
    c3.onmessage = handler;

    c1.postMessage('from c1');
    c3.postMessage('from c3');
    c2.postMessage('done');
  }, 'messages are delivered in port creation order');



Actual results:

Most of the time the test fails (but sometimes it passes). 


Expected results:

The test should pass. The specification for BroadcastChannel [1] says that all BroadcastChannel instances with the same event loop should be sorted in creation order when determining which order to deliver events. The order in firefox appears to be random.

[1]: https://html.spec.whatwg.org/multipage/comms.html#dom-broadcastchannel-postmessage

Comment 1

[Andrea Marchesini [:baku]]

Attachment: bc2.patch

Comment 2

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

Comment on attachment 8754793 [details] [diff] [review]
bc2.patch

>+  // Ricreate the BlobParent for this new message.
Ricreate?


>+  // Raw Pointers because the actors keep alive this service.
>+  nsDataHashtable<nsStringHashKey,
>+                 nsTArray<BroadcastChannelParent*>*> mAgents;
nit, missing a space before nsTArray




>@@ -466,17 +466,26 @@ BackgroundParentImpl::AllocPBroadcastCha
>                                             const PrincipalInfo& aPrincipalInfo,
>                                             const nsCString& aOrigin,
>                                             const nsString& aChannel,
>                                             const bool& aPrivateBrowsing)
> {
>   AssertIsInMainProcess();
>   AssertIsOnBackgroundThread();
> 
>-  return new BroadcastChannelParent(aOrigin, aChannel, aPrivateBrowsing);
>+  nsString originChannelKey;
>+
>+  if (aPrivateBrowsing) {
>+    originChannelKey.AppendLiteral("PB-");
>+  }
>+
>+  originChannelKey.Append(NS_ConvertUTF8toUTF16(aOrigin));
>+  originChannelKey.Append(aChannel);
>+
>+  return new BroadcastChannelParent(originChannelKey);

So "PB-" prefix isn't safe here. Please use similar suffix what origin attributes have.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: bc2.patch

Comment 4

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

Comment on attachment 8754835 [details] [diff] [review]
bc2.patch

>+++ b/ipc/glue/BackgroundParentImpl.cpp
>@@ -466,17 +466,30 @@ BackgroundParentImpl::AllocPBroadcastCha
>                                             const PrincipalInfo& aPrincipalInfo,
>                                             const nsCString& aOrigin,
>                                             const nsString& aChannel,
>                                             const bool& aPrivateBrowsing)
> {
>   AssertIsInMainProcess();
>   AssertIsOnBackgroundThread();
> 
>-  return new BroadcastChannelParent(aOrigin, aChannel, aPrivateBrowsing);
>+  nsString originChannelKey;
>+
>+  // The format of originChannelKey is:
>+  //  <channelName>|<origin+OriginAttributes>&privateBrowsing=true
This is still problematic, since originattributes is possibly empty, so &privateBrowsing=true becomes part of the origin, and 
(based on #whatwg) at least & can be part of the origin.
Please fix, or explain why this setup is safe.

Comment 5

[Andrea Marchesini [:baku]]

Let's use '|' + privateBrowsing=true
Eventually privateBrowsing will be part of the OriginAttributes, but for now, this seems a good solution to me.

Comment 6

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

At least Gecko's url parsing accepts |.
(new URL("https://test|test")).origin == "https://test|test"
so | isn't good enough.

Comment 7

[Andrea Marchesini [:baku]]

Then I suggest: <channelName> + "|" + privateBrowsing=true/false + "|" + origin

Comment 8

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

I guess that works

Comment 9

[Andrea Marchesini [:baku]]

Attachment: bc2.patch

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0659c9235136

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df5daa1095f2

Comment 12

[Carsten Book [:Tomcat]]

backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=28515069&repo=mozilla-inbound

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51cc934506df

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51cc934506df