Closed Bug 1274363 Opened 3 years ago Closed 3 years ago

BroadcastChannel dispatches event in wrong order

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mek, Assigned: baku)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 2 obsolete files)

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
Flags: needinfo?(amarchesini)
Attached patch bc2.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(amarchesini)
Attachment #8754793 - Flags: review?(bugs)
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.
Attachment #8754793 - Flags: review?(bugs) → review-
Attached patch bc2.patch (obsolete) — Splinter Review
Attachment #8754793 - Attachment is obsolete: true
Attachment #8754835 - Flags: review?(bugs)
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.
Attachment #8754835 - Flags: review?(bugs) → review-
Let's use '|' + privateBrowsing=true
Eventually privateBrowsing will be part of the OriginAttributes, but for now, this seems a good solution to me.
Flags: needinfo?(bugs)
At least Gecko's url parsing accepts |.
(new URL("https://test|test")).origin == "https://test|test"
so | isn't good enough.
Flags: needinfo?(bugs)
Then I suggest: <channelName> + "|" + privateBrowsing=true/false + "|" + origin
Flags: needinfo?(bugs)
I guess that works
Flags: needinfo?(bugs)
Attached patch bc2.patchSplinter Review
Attachment #8754835 - Attachment is obsolete: true
Attachment #8755413 - Flags: review?(bugs)
Attachment #8755413 - Flags: review?(bugs) → review+
backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=28515069&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Whiteboard: btpp-active
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/51cc934506df
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.