1258600 - [Presentation WebAPI] implement PresentationConnectionClosedEvent

Status: RESOLVED FIXED

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

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

PresentationConnectionClosedEvent is introduced to identify the reason of connection closing.

Comment 1

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

Looks like the event interface could be implemented using event codegen.
Just add the .webidl interface and then the file to GENERATED_EVENTS_WEBIDL_FILES list in moz.build.
After building .cpp file will be in <objdir>/dom/bindings/<the_new_event>.cpp and .h
file in <objdir>/dist/include/mozilla/dom/<the_new_event>.h

Filed https://github.com/w3c/presentation-api/issues/273
I assume .message should have default value "" in dictionary.

Comment 2

[Andrew Overholt [:overholt]]

(I'm marking this as "fixlater" but feel free to change that if you're going to work on this sooner (or later :), SC)

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Implement PresentationConnectionClosedEvent - v1

First version.
Please take a quick look. Thanks.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8752026 [details] [diff] [review]
Implement PresentationConnectionClosedEvent - v1

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

We'll need to send the close reason to remote device to implement the wentaway behavior. If it is not going to be done in this bug, you'll need to define the scope in bugzilla comment and file a dependent bug.

::: dom/presentation/PresentationConnection.cpp
@@ +110,5 @@
> +    mState = PresentationConnectionState::Closed;
> +    DispatchConnectionClosedEvent(PresentationConnectionClosedReason::Wentaway,
> +                                  EmptyString());
> +  }
> +

This modification doesn't matched with https://w3c.github.io/presentation-api/#closing-a-presentationconnection.
For page that navigated away it should shutdown the connection with reason "wentaway" (so that remote page will receive close event with reason "wentaway") but not dispatch close event locally.

@@ +217,5 @@
>    }
>  
>    mState = state;
>  
> +  if (mState == PresentationConnectionState::Connected) {

We can move the entire section into another function.

@@ +237,5 @@
> +                                                       message))) {
> +        mozilla::GetErrorName(aReason, message);
> +        message.InsertLiteral("Internal error: ", 0);
> +      }
> +      CopyASCIItoUTF16(message, errorMsg);

should use |CopyUTF8toUTF16|.

@@ +291,5 @@
>    return DispatchMessageEvent(jsData);
>  }
>  
>  nsresult
> +PresentationConnection::DispatchStateChangeEvent(const nsAString& aEventType)

Should either change the function name to something meaningful or use AsyncEventDispatcher directly without this helper function.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8752026 [details] [diff] [review]
> Implement PresentationConnectionClosedEvent - v1
> 
> Review of attachment 8752026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We'll need to send the close reason to remote device to implement the
> wentaway behavior. If it is not going to be done in this bug, you'll need to
> define the scope in bugzilla comment and file a dependent bug.
> 

According to [1], when receiving a signal from the destination browsing context that a connection is going to be closed, the close reason can be either closed or wentaway.
So, I plan to just close the transport channel when the PresentationConnection is discarded by browser. At the other side, the destination browser can only close the connection with "closed" reason.

[1] https://w3c.github.io/presentation-api/#interface-presentationconnection

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionClosedEvent

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement onconnect, onclose and onterminate event handlers

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionClosedEvent

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement onconnect, onclose and onterminate event handlers - v2

Rebase on bug 1258602.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case changes

1. Replace |onstatechange| for current test cases.
2. Add a new test for testing when a connection is went away.

Comment 11

[Kershaw Chang [:kershaw]]

Comment on attachment 8754333 [details] [diff] [review]
Part3: Test case changes

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

::: dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway.js
@@ +138,5 @@
> +      aResolve();
> +    };
> +    gScript.addMessageListener('ready-to-remove-receiverFrame', function onReadyToRemove() {
> +      gScript.removeMessageListener('ready-to-remove-receiverFrame', onReadyToRemove);
> +      document.body.removeChild(receiverIframe);

It seems this is the only way to trigger |DisconnectFromOwner| at receiver side. I tried to modify receiverFrame.src, but not working.

Comment 12

[Kershaw Chang [:kershaw]]

Comment on attachment 8754332 [details] [diff] [review]
Part2: Implement onconnect, onclose and  onterminate event handlers - v2

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

::: dom/presentation/PresentationConnection.cpp
@@ +116,5 @@
> +
> +  nsresult rv = service->CloseSession(
> +    mId, mRole, nsIPresentationService::CLOSED_REASON_WENTAWAY);
> +  NS_WARN_IF(NS_FAILED(rv));
> +

According to spec, when the browsing context was navigated or was discarded, we should close the connection with the reason wentaway.

Is DisconnectFromOwner a good place for closing the connection? Or should I listen to OnLocationChange?

Comment 13

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

Do we somewhere block pages which use presentation API to enter bfache? If not, then DisconnectFromOwner certainly won't catch "browsing context was navigated" case.
Could presentation session extend nsIRequest and add itself to relevant loadgroup, and then when loadgroup is "stopped", it would get a notification.

Comment 14

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

Comment on attachment 8754332 [details] [diff] [review]
Part2: Implement onconnect, onclose and  onterminate event handlers - v2

So I think nsIRequest approach probably the easiest for this stuff.
It also deals with bfcache automatically, 
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=1fcf8a5711ed&mark=8888-8888,8908-8943#8888
so that if there are requests in the loadgroup, we don't enter to bfcache.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement onconnect, onclose and onterminate event handlers - v3

Make PresentationConnection to extend nsIRequest and add itself to the load group.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case changes

Change receiver frame's src to test wentaway.

Comment 17

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

Comment on attachment 8754737 [details] [diff] [review]
Part2: Implement onconnect, onclose and  onterminate event handlers - v3

>+NS_IMETHODIMP
>+PresentationConnection::Cancel(nsresult aStatus)
>+{
>+  return ProcessConnectionWentAway();
Could we process ProcessConnectionWentAway asynchronously.
Documentation for nsIRequest::Cancel is pretty strict that nothing unexpected should happen inside the method.
So,
nsCOMPtr<nsIRunnable> event =
  NewRunnableMethod(this, &PresentationConnection::ProcessConnectionWentAway);
NS_DispatchToCurrentThread(ev);

Ok, I see PresentationConnection::Close() hasn't been implemented yet, since I'd expect that to close the connection properly and remove
the connection from loadgroup.
I guess that will happen in some other bug.

Comment 18

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

Comment on attachment 8754738 [details] [diff] [review]
Part3: Test case changes

Mostly rs+

Comment 19

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

Not necessarily about this bug, but asking anyhow (since the loadgroup->AddRequest reminded me)
Is it defined somewhere what kind of lifetime management PresentationConnection should have?
I mean, if it doesn't have any event listeners and JS doesn't have a reference to it, should the connection be closed automatically and GC/CC collect the object?

Loadgroup does keep a strong reference to the requests, so the patches do effectively mean that connecion stays alive as long as the page is alive. Maybe that is ok?

Comment 20

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #19)
> Not necessarily about this bug, but asking anyhow (since the
> loadgroup->AddRequest reminded me)
> Is it defined somewhere what kind of lifetime management
> PresentationConnection should have?
> I mean, if it doesn't have any event listeners and JS doesn't have a
> reference to it, should the connection be closed automatically and GC/CC
> collect the object?
> 

It seems that the lifetime of PresentationConnection is not defined in spec or any other places.
Maybe S.C. knows?

> Loadgroup does keep a strong reference to the requests, so the patches do
> effectively mean that connecion stays alive as long as the page is alive.
> Maybe that is ok?

I think if the PresentationConnection is not used anywhere in js, it should be released automatically.
Would it be better if we do loadgroup->AddRequest when the connection becomes closed or terminated?

Comment 21

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

I think there are two things to consider to do. These are not either-or, but separate things.

(1) WebSocket for example keeps itself alive if there are event listeners for message or close events (you may want to read what HTML spec says about it). See also WebSocket::UpdateMustKeepAlive(). PresentationConnection could do something similar.
I think the same setup is being used also elsewhere... was it in WebRTC or WebAudio or where...


(2) To not depend on loadgroup's strong references, I would call AddRequest when the connection is created, but to effectively make the loadgroug->connection edge "weak", in Traverse method
do something like
  NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "self");
  cb.NoteXPCOMChild(tmp);
whenever we haven't yet called RemoveRequest.
That way cycle collector wouldn't need to know about the loadgroup->connection edge, since it has this artificial "self" edge.

Comment 22

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #21)
> I think there are two things to consider to do. These are not either-or, but
> separate things.
> 
> (1) WebSocket for example keeps itself alive if there are event listeners
> for message or close events (you may want to read what HTML spec says about
> it). See also WebSocket::UpdateMustKeepAlive(). PresentationConnection could
> do something similar.
> I think the same setup is being used also elsewhere... was it in WebRTC or
> WebAudio or where...
> 
> 
> (2) To not depend on loadgroup's strong references, I would call AddRequest
> when the connection is created, but to effectively make the
> loadgroug->connection edge "weak", in Traverse method
> do something like
>   NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "self");
>   cb.NoteXPCOMChild(tmp);
> whenever we haven't yet called RemoveRequest.
> That way cycle collector wouldn't need to know about the
> loadgroup->connection edge, since it has this artificial "self" edge.

Thanks for the suggestions.
If you agree, I would like to file a new bug for doing this.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The lifecycle of PresentationConnection might be different between controller and receiver.

For controller, the lifecycle should be similar to WebSocket since it is initiated by controller page. Closing connection if no JS reference to it sounds reasonable.

For receiver, the connection is associated with a global `receiver` object. Therefore, connection should be kept as long as the page is alive.

Comment 24

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cda8aac0b04

Comment 25

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add PresentationConnectionClosedEvent, r=smaug

Rebase and carry r+.

Comment 26

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implement onconnect, onclose and onterminate event handlers, r=smaug

Comment 27

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case changes, r=smaug

Comment 28

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94c4c36af2a8

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a6b93fbd16
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3fef6d6d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7836822d084

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47a6b93fbd16
https://hg.mozilla.org/mozilla-central/rev/3f3fef6d6d9c
https://hg.mozilla.org/mozilla-central/rev/b7836822d084

Comment 31

[Kershaw Chang [:kershaw]]

Please help to approve. Thanks.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Presentation API
User impact if declined: PresentationConnnectionClosedEvent would not be available.
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 32

[Josh Cheng [:josh]]

Comment on attachment 8757977 [details] [diff] [review]
Part1: Add PresentationConnectionClosedEvent, r=smaug

Approve for TV 2.6

Comment 33

[Josh Cheng [:josh]]

Comment on attachment 8757978 [details] [diff] [review]
Part2: Implement onconnect, onclose and  onterminate event handlers, r=smaug

Approve for TV 2.6

Comment 34

[Josh Cheng [:josh]]

Comment on attachment 8757979 [details] [diff] [review]
Part3: Test case changes, r=smaug

Approve for TV 2.6

Comment 35

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/8f0ee7349702af19342f6c172f4f371852e4bec5