Closed Bug 1153134 Opened 9 years ago Closed 8 years ago

[Presentation WebAPI] TLS control channel

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Project Flags:
blocking-b2g 2.6+
Tracking Flags:
Tracking Status
b2g-v2.6 --- fixed
firefox51 --- fixed

People

(Reporter: CuveeHsu, Assigned: schien)

References

Details

(Whiteboard: [ETA 7/15])

Attachments

(4 files, 12 obsolete files)

part 1, check-server-tls-support.patch
(deleted), patch
schien
: review+
Details | Diff | Splinter Review
part 2, support-tls-control-server.patch. v6
(deleted), patch
schien
: review+
Details | Diff | Splinter Review
part 3, retry-start-server-after-timeout.patch
(deleted), patch
schien
: review+
Details | Diff | Splinter Review
pull request for tv 2.6
(deleted), text/x-github-pull-request
jocheng
: approval-mozilla-b2g48+
Details 
We have a TCP control channel without encryption, which might be vulnerable.
A plain TCP control channel may leak seesion ID and some personal information. I'm wondering if TLS is not enough to prevent hacking, such as replay attack. Maybe we need nonce or another offer-answer model?

Fabrice, sorry to bother you continuously >"<
Do you have any idea or who should I need info from?
Moreover, should it be under security review?
Flags: needinfo?(fabrice)
Junior, let's see what the security folks want.
Flags: needinfo?(fabrice) → needinfo?(ptheriault)
Just FYI that Stephanie and I are looking at this. Junior, I'll be in touch.
Flags: needinfo?(ptheriault)
blocking-b2g: --- → 2.6?
Whiteboard: [ETA 6/30]
I plan to follow the design that TV Remote Control used [1] for authentication and data encryption. I put details on https://wiki.mozilla.org/WebAPI/PresentationAPI:Protocol_Draft.

[1] https://wiki.mozilla.org/Firefox_OS/Remote_Control#Architecture_Designs
blocking-b2g: 2.6? → 2.6+
Whiteboard: [ETA 6/30] → [ETA 7/15]
Assignee: nobody → schien
Attached patch [WIP] part 1, check-server-tls-support.patch (obsolete) (deleted) — — Splinter Review 
Controller will get the server certificate from mDNS record and create TLS socket while establishing control channel.
Attached patch [WIP] part 2, support-tls-control-server.patch (obsolete) (deleted) — — Splinter Review 
generate self-signed certificate and broadcast the fingerprint over mDNS.
I use the same mechanism as in flyweb. Server will use a self-signed certificate and broadcast the SHA256 fingerprint via mDNS. Client will use the SHA256 fingerprint to temporary allow the certificate on designated host and port while creating TLS socket.
Attached patch part 1, check-server-tls-support.patch (obsolete) (deleted) — — Splinter Review 
Use the SHA256 certificate fingerprint in mDNS service record to establish TLS connection.
Attachment #8772687 - Attachment is obsolete: true
Attachment #8774627 - Flags: review?(juhsu)
Attached patch part 2, support-tls-control-server.patch (obsolete) (deleted) — — Splinter Review 
This patch consist of following modifications:
1. make nsIPresentationControlService.startServer async because TLS certificate generation is async only.
2. make TCPControlChannel.send async because the first message send on output stream of TLS socket might fail during the handshake procedure.
3. allow DISCONNECT message in closing state due to the natural of async send
4. use a guard timer to guarantee TERMINATE_ACK is received.
5. use a guard timer to guarantee IO stream is closed after DISCONNECT procedure is triggered.
Attachment #8772688 - Attachment is obsolete: true
Attachment #8774632 - Flags: review?(juhsu)
This bug depends on nsICertOverrideService.rememberTemporaryValidityOverrideUsingFingerprint, which is introduced by bug 1275714. We'll need to uplift bug 1275714 on tv 2.6 as well.
Depends on: 1275714
Plan to review this within 24 hours.
Comment on attachment 8774627 [details] [diff] [review]
part 1, check-server-tls-support.patch

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

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +19,5 @@
>  {
>    readonly attribute AUTF8String id;
>    readonly attribute AUTF8String address;
>    readonly attribute uint16_t port;
> +  readonly attribute AUTF8String certFingerprint;

Should we force the Device always use TLS in the future?
If not, annotate the intuition of null certFingerprint.

::: dom/presentation/provider/PresentationControlService.js
@@ +32,3 @@
>    this.address = aAddress;
>    this.port = aPort;
>    this.id = aId;

why not save aCertFingerprint?
Attachment #8774627 - Flags: review?(juhsu) → review+
Comment on attachment 8774627 [details] [diff] [review]
part 1, check-server-tls-support.patch

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

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +19,5 @@
>  {
>    readonly attribute AUTF8String id;
>    readonly attribute AUTF8String address;
>    readonly attribute uint16_t port;
> +  readonly attribute AUTF8String certFingerprint;

for 1-UA case TLS is not necessary because it's a local connection. I'll add a comment about "empty string means device doesn't support TLS".

::: dom/presentation/provider/PresentationControlService.js
@@ +32,3 @@
>    this.address = aAddress;
>    this.port = aPort;
>    this.id = aId;

Thanks for catching this, however no behavior change since we don't generate client cert.
Comment on attachment 8774632 [details] [diff] [review]
part 2, support-tls-control-server.patch

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

Cancel the review because I need more information to continue the progress.

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +31,4 @@
>     * @param   aPort
>     *          The port of the server socket.
> +   * @param   aCertFingerprint
> +   *          The fingerprint of TLS server certificate.

Annotate what null aCertFingerprint means.

@@ +39,5 @@
> +   * Callback while server is stopped.
> +   * @param   aRv
> +   *          The error cause of server stopped.
> +   */
> +  void onServerStopped(in nsresult aRv);

I won't insist,
but I think |aResult| is more common and also in those in .cpp

@@ +75,5 @@
>  interface nsIPresentationControlService: nsISupports
>  {
>    /**
> +   * This method initialize server socket. Caller should set listener and
> +   * monitor onServerReady event to get the correct server info.

Since |startServer| is coming to be async, how if |startServer| fails?
Is the |onServerStopped| called? Annotate this situation.

Could we provide a method like |cancel| if the caller feels it's too long for a callback?

@@ +109,5 @@
>    void close();
>  
>    /**
> +   * Get the listen port of the TCP socket, valid after server is ready.
> +   * 0 indicates the server socket is not ready or closed.

1. valid after the server is ready
2. Maybe s/not ready/unready/ to remove ambiguity

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +75,5 @@
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore unexpected command.
> +        break;
> +    }

I'm not sure the reason of this change. Why TLS support has impact of this behavior?

::: dom/presentation/provider/PresentationControlService.js
@@ +32,5 @@
>  function TCPDeviceInfo(aAddress, aPort, aId, aCertFingerprint) {
>    this.address = aAddress;
>    this.port = aPort;
>    this.id = aId;
> +  this.certFingerprint = aCertFingerprint;

Okay I find this here.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +395,5 @@
>    }
>  }
>  
>  
>  function shutdown()

There's another |onPortChange| in shutdown. You should substitute it to |OnServerReady|
Attachment #8774632 - Flags: review?(juhsu)
Attached patch part 1, check-server-tls-support.patch (obsolete) (deleted) — — Splinter Review 
update according to review comment, carry r+.
Attachment #8774627 - Attachment is obsolete: true
Attachment #8775434 - Flags: review+
Comment on attachment 8774632 [details] [diff] [review]
part 2, support-tls-control-server.patch

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

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +31,4 @@
>     * @param   aPort
>     *          The port of the server socket.
> +   * @param   aCertFingerprint
> +   *          The fingerprint of TLS server certificate.

Done.

@@ +39,5 @@
> +   * Callback while server is stopped.
> +   * @param   aRv
> +   *          The error cause of server stopped.
> +   */
> +  void onServerStopped(in nsresult aRv);

Done.

@@ +75,5 @@
>  interface nsIPresentationControlService: nsISupports
>  {
>    /**
> +   * This method initialize server socket. Caller should set listener and
> +   * monitor onServerReady event to get the correct server info.

|onServerStopped| will be called if fail to start server. I'll add it in comment.

Caller can always use |nsIPresentationControlService.close| to cancel the start procedure so I think there is no need to introduce another |cancel| function.

@@ +109,5 @@
>    void close();
>  
>    /**
> +   * Get the listen port of the TCP socket, valid after server is ready.
> +   * 0 indicates the server socket is not ready or closed.

1. Done.
2. s/or closed/or is closed/

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +75,5 @@
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore unexpected command.
> +        break;
> +    }

This change is to compensate the async write modification.
After termination, both end points will initiate the disconnect procedure. In our protocol design, device will wait for IO stream close after sending DISCONNECT command. So, we'll need to handle the situation when both end points initiate DISCONNECT procedure to close the IO stream earlier instead of waiting for the timeout of DISCONNECT guard timer.

::: dom/presentation/provider/PresentationControlService.js
@@ +32,5 @@
>  function TCPDeviceInfo(aAddress, aPort, aId, aCertFingerprint) {
>    this.address = aAddress;
>    this.port = aPort;
>    this.id = aId;
> +  this.certFingerprint = aCertFingerprint;

I'll move this modification to part 1.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +395,5 @@
>    }
>  }
>  
>  
>  function shutdown()

Done.
Attached patch part 2, support-tls-control-server.patch. v2 (obsolete) (deleted) — — Splinter Review 
update according to previous review comment.
Attachment #8774632 - Attachment is obsolete: true
Attachment #8775463 - Flags: review?(juhsu)
Comment on attachment 8775463 [details] [diff] [review]
part 2, support-tls-control-server.patch. v2

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

Cancel the review because I still have some questions.
Feel free to ask review again after clarification.

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +38,5 @@
>     */
> +  void onServerReady(in uint16_t aPort, in AUTF8String aCertFingerprint);
> +
> +  /**
> +   * Callback while server is stopped or fail to start.

Callback while _the_ server is stopped or _fails_ to start.

@@ +41,5 @@
> +  /**
> +   * Callback while server is stopped or fail to start.
> +   * @param   aResult
> +   *          The error cause of server stopped or the reason of
> +   *          start failure.

You should mention the NS_OK case.

@@ +78,5 @@
>  [scriptable, uuid(55d6b605-2389-4aae-a8fe-60d4440540ea)]
>  interface nsIPresentationControlService: nsISupports
>  {
>    /**
> +   * This method initialize server socket. Caller should set listener and

initializes

::: dom/presentation/provider/DisplayDeviceProvider.cpp
@@ +381,5 @@
> +
> +  // Try restart server if it is stopped abnormally.
> +  if (NS_FAILED(aResult)) {
> +    return NS_DispatchToMainThread(
> +             NewRunnableMethod(this, &DisplayDeviceProvider::StartTCPService));

A chance to have infinite function calls here.
And why we need to set every parameter again instead of just |StartServer|?

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +219,5 @@
>      return rv;
>    }
>  
>    /**
>      * If |servicePort| is non-zero, it means PresentationServer is running.

s/PresentationServer/PresentationControlService

@@ +945,5 @@
> +
> +  // Try restart server if it is stopped abnormally.
> +  if (NS_FAILED(aResult) && mDiscoverable) {
> +    return NS_DispatchToMainThread(
> +             NewRunnableMethod(this, &MulticastDNSDeviceProvider::StartServer));

I'm worried if there's a infinite call sequence here.
How do you think?

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +152,2 @@
>    nsresult RegisterService();
>    nsresult UnregisterService(nsresult aReason);

Annotate the above four are about PresentationControlService, or reveal this in the function name

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +28,5 @@
>  versionAttr.setPropertyAsUint32("version", LATEST_VERSION);
>  
>  var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
>  
> +function sleep(ms) {

aMs

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +67,5 @@
>  
> +  // First run with TLS enabled.
> +  pcs.startServer(true, PRESENTER_CONTROL_CHANNEL_PORT);
> +  if (pcs.port !== 0) {
> +    testPresentationServer();

I'm confused about two |testPresentationServer| here. Since |startServer| becomes async, every method except |close| for pcs should called after the callbacks.

It seems |startServer| becomes sync if we can get a non-zero port.

Moreover, if the port access is guaranteed, annotate this in the idl.
Attachment #8775463 - Flags: review?(juhsu)
Comment on attachment 8775463 [details] [diff] [review]
part 2, support-tls-control-server.patch. v2

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

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +38,5 @@
>     */
> +  void onServerReady(in uint16_t aPort, in AUTF8String aCertFingerprint);
> +
> +  /**
> +   * Callback while server is stopped or fail to start.

done.

@@ +41,5 @@
> +  /**
> +   * Callback while server is stopped or fail to start.
> +   * @param   aResult
> +   *          The error cause of server stopped or the reason of
> +   *          start failure.

done.

@@ +78,5 @@
>  [scriptable, uuid(55d6b605-2389-4aae-a8fe-60d4440540ea)]
>  interface nsIPresentationControlService: nsISupports
>  {
>    /**
> +   * This method initialize server socket. Caller should set listener and

done.

::: dom/presentation/provider/DisplayDeviceProvider.cpp
@@ +381,5 @@
> +
> +  // Try restart server if it is stopped abnormally.
> +  if (NS_FAILED(aResult)) {
> +    return NS_DispatchToMainThread(
> +             NewRunnableMethod(this, &DisplayDeviceProvider::StartTCPService));

I thought about that while writing this patch. It might have infinite async function call, but I'm more afraid about silently stop retry with no UI indication. We can have a quick chat about this to see if any other solution.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +219,5 @@
>      return rv;
>    }
>  
>    /**
>      * If |servicePort| is non-zero, it means PresentationServer is running.

done.

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +152,2 @@
>    nsresult RegisterService();
>    nsresult UnregisterService(nsresult aReason);

|StartServer| and |StopServer| is quite straight forward so I don't think it's necessary to rename or add any comment.

|RegisterService|/|UnregisterService| is for registering/unregistering presentation control server as an mDNS service. Do you think rename it to |RegisterMDNSService|/|UnregisterMDNSService| will make it clear enough?

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +28,5 @@
>  versionAttr.setPropertyAsUint32("version", LATEST_VERSION);
>  
>  var registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);
>  
> +function sleep(ms) {

done.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +67,5 @@
>  
> +  // First run with TLS enabled.
> +  pcs.startServer(true, PRESENTER_CONTROL_CHANNEL_PORT);
> +  if (pcs.port !== 0) {
> +    testPresentationServer();

Sorry this is code should be removed. It is just a intermediary modification before I make startServer purely async.
ni? @junior to look at comment #19
Flags: needinfo?(juhsu)
> ::: dom/presentation/provider/DisplayDeviceProvider.cpp
> @@ +381,5 @@
> > +
> > +  // Try restart server if it is stopped abnormally.
> > +  if (NS_FAILED(aResult)) {
> > +    return NS_DispatchToMainThread(
> > +             NewRunnableMethod(this, &DisplayDeviceProvider::StartTCPService));
> 
> I thought about that while writing this patch. It might have infinite async
> function call, but I'm more afraid about silently stop retry with no UI
> indication. We can have a quick chat about this to see if any other solution.
> 
> 

It's tough, but infinite async calls will hang the app. 
At least we have a sleep, with constant or incremental time interval, to avoid hanging,
but it's not a good-enough solution.

For providers, if user feels that the list should have contained some device,
I guess we'll have a button to refresh and call |forceRecovery|.
Then we have a chance for user to startServer and notice some error.

But we can land a version with retry after sleep.

> ::: dom/presentation/provider/MulticastDNSDeviceProvider.h
> @@ +152,2 @@
> >    nsresult RegisterService();
> >    nsresult UnregisterService(nsresult aReason);
> 
> |StartServer| and |StopServer| is quite straight forward so I don't think
> it's necessary to rename or add any comment.
> 
> |RegisterService|/|UnregisterService| is for registering/unregistering
> presentation control server as an mDNS service. Do you think rename it to
> |RegisterMDNSService|/|UnregisterMDNSService| will make it clear enough?
> 
Okay let's do it.
Flags: needinfo?(juhsu)
(In reply to Junior [:junior] from comment #21)
> > ::: dom/presentation/provider/DisplayDeviceProvider.cpp
> > @@ +381,5 @@
> > > +
> > > +  // Try restart server if it is stopped abnormally.
> > > +  if (NS_FAILED(aResult)) {
> > > +    return NS_DispatchToMainThread(
> > > +             NewRunnableMethod(this, &DisplayDeviceProvider::StartTCPService));
> > 
> > I thought about that while writing this patch. It might have infinite async
> > function call, but I'm more afraid about silently stop retry with no UI
> > indication. We can have a quick chat about this to see if any other solution.
> > 
> > 
> 
> It's tough, but infinite async calls will hang the app. 
> At least we have a sleep, with constant or incremental time interval, to
> avoid hanging,
> but it's not a good-enough solution.
> 
> For providers, if user feels that the list should have contained some device,
> I guess we'll have a button to refresh and call |forceRecovery|.
> Then we have a chance for user to startServer and notice some error.
> 
> But we can land a version with retry after sleep.
I'll add the retry-after-timeout in patch part 3.
> 
> > ::: dom/presentation/provider/MulticastDNSDeviceProvider.h
> > @@ +152,2 @@
> > >    nsresult RegisterService();
> > >    nsresult UnregisterService(nsresult aReason);
> > 
> > |StartServer| and |StopServer| is quite straight forward so I don't think
> > it's necessary to rename or add any comment.
> > 
> > |RegisterService|/|UnregisterService| is for registering/unregistering
> > presentation control server as an mDNS service. Do you think rename it to
> > |RegisterMDNSService|/|UnregisterMDNSService| will make it clear enough?
> > 
> Okay let's do it.

Done.
Attached patch part 2, support-tls-control-server.patch. v3 (obsolete) (deleted) — — Splinter Review 
update according to review comment. put retry after timeout in part 3.
Attachment #8775463 - Attachment is obsolete: true
Attachment #8776462 - Flags: review?(juhsu)
Comment on attachment 8776462 [details] [diff] [review]
part 2, support-tls-control-server.patch. v3

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

Cancel the review since I'd like to confirm if the test pass after removing the sync call in |test_tcp_control_channel.js|

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +38,5 @@
>     */
> +  void onServerReady(in uint16_t aPort, in AUTF8String aCertFingerprint);
> +
> +  /**
> +   * Callback while the server is stopped or fail to start.

nit: s/fail/fails

::: dom/presentation/provider/PresentationControlService.js
@@ +66,5 @@
> +      let self = this;
> +      let localCertService = Cc["@mozilla.org/security/local-cert-service;1"]
> +                               .getService(Ci.nsILocalCertService);
> +      localCertService.getOrCreateCert(kLocalCertName, {
> +        handleCert: function(aCert, aRv) {

Can we use arrow function to avoid using self trick?

@@ +125,5 @@
> +
> +  _notifyServerReady: function() {
> +    Services.tm.mainThread.dispatch(() => {
> +      if (this._listener) {
> +        this._listener.onServerReady(this._port, this.certFingerprint);

Is there any chance that we have a null-listener? Same as _notifyServerStopped

@@ +436,5 @@
>  
> +const kDisconnectTimeout = 5000;
> +const kTerminateTimeout = 5000;
> +
> +function setTimeout(callback, delay) {

How about using Timer.jsm?

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +307,5 @@
> +    deferred.promise,
> +    sleep(1000),
> +  ]);
> +
> +  race.then(()=>{

nit: space around arrow.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +67,5 @@
>  
> +  // First run with TLS enabled.
> +  pcs.startServer(true, PRESENTER_CONTROL_CHANNEL_PORT);
> +  if (pcs.port !== 0) {
> +    testPresentationServer();

Confused dup |testPresentationServer|s

@@ +386,5 @@
> +
> +    // Second run with TLS disabled.
> +    pcs.startServer(false, PRESENTER_CONTROL_CHANNEL_PORT);
> +    if (pcs.port !== 0) {
> +      testPresentationServer();

same here
Attachment #8776462 - Flags: review?(juhsu)
Comment on attachment 8776462 [details] [diff] [review]
part 2, support-tls-control-server.patch. v3

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

::: dom/presentation/interfaces/nsIPresentationControlService.idl
@@ +38,5 @@
>     */
> +  void onServerReady(in uint16_t aPort, in AUTF8String aCertFingerprint);
> +
> +  /**
> +   * Callback while the server is stopped or fail to start.

done.

::: dom/presentation/provider/PresentationControlService.js
@@ +66,5 @@
> +      let self = this;
> +      let localCertService = Cc["@mozilla.org/security/local-cert-service;1"]
> +                               .getService(Ci.nsILocalCertService);
> +      localCertService.getOrCreateCert(kLocalCertName, {
> +        handleCert: function(aCert, aRv) {

No, we cannot do that because nsILocalCertGetCallback is not a function interface.

@@ +125,5 @@
> +
> +  _notifyServerReady: function() {
> +    Services.tm.mainThread.dispatch(() => {
> +      if (this._listener) {
> +        this._listener.onServerReady(this._port, this.certFingerprint);

There is no guarantee that all the tasks in event queue will not unset listener.

@@ +436,5 @@
>  
> +const kDisconnectTimeout = 5000;
> +const kTerminateTimeout = 5000;
> +
> +function setTimeout(callback, delay) {

done.

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +307,5 @@
> +    deferred.promise,
> +    sleep(1000),
> +  ]);
> +
> +  race.then(()=>{

done.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +67,5 @@
>  
> +  // First run with TLS enabled.
> +  pcs.startServer(true, PRESENTER_CONTROL_CHANNEL_PORT);
> +  if (pcs.port !== 0) {
> +    testPresentationServer();

done.

@@ +386,5 @@
> +
> +    // Second run with TLS disabled.
> +    pcs.startServer(false, PRESENTER_CONTROL_CHANNEL_PORT);
> +    if (pcs.port !== 0) {
> +      testPresentationServer();

done.
Attached patch part 2, support-tls-control-server.patch. v4 (obsolete) (deleted) — — Splinter Review 
update according to review comments.
Attachment #8776462 - Attachment is obsolete: true
Attachment #8776547 - Flags: review?(juhsu)
> @@ +125,5 @@
> > +
> > +  _notifyServerReady: function() {
> > +    Services.tm.mainThread.dispatch(() => {
> > +      if (this._listener) {
> > +        this._listener.onServerReady(this._port, this.certFingerprint);
> 
> There is no guarantee that all the tasks in event queue will not unset
> listener.
> 
Okay, so the question is:
If this._listener is null when _notifyServerReady is called, 
should we call |onServerReady| once the listener is set?
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #24)
> Created attachment 8776463 [details] [diff] [review]
> part 3, retry-start-server-after-timeout.patch

How about |DisplayDeviceProvider|?
ni for last two comments
Flags: needinfo?(schien)
(In reply to Junior [:junior] from comment #28)
> > @@ +125,5 @@
> > > +
> > > +  _notifyServerReady: function() {
> > > +    Services.tm.mainThread.dispatch(() => {
> > > +      if (this._listener) {
> > > +        this._listener.onServerReady(this._port, this.certFingerprint);
> > 
> > There is no guarantee that all the tasks in event queue will not unset
> > listener.
> > 
> Okay, so the question is:
> If this._listener is null when _notifyServerReady is called, 
> should we call |onServerReady| once the listener is set?

We can enforce that listener needs to be provided before doing startServer/close and document it in IDL. This assumption is already existed in all of our device provider implementation and I don't really want to change it.
Flags: needinfo?(schien)
(In reply to Junior [:junior] from comment #29)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #24)
> > Created attachment 8776463 [details] [diff] [review]
> > part 3, retry-start-server-after-timeout.patch
> 
> How about |DisplayDeviceProvider|?

Will provide in next version.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #31)
> (In reply to Junior [:junior] from comment #28)
> > > @@ +125,5 @@
> > > > +
> > > > +  _notifyServerReady: function() {
> > > > +    Services.tm.mainThread.dispatch(() => {
> > > > +      if (this._listener) {
> > > > +        this._listener.onServerReady(this._port, this.certFingerprint);
> > > 
> > > There is no guarantee that all the tasks in event queue will not unset
> > > listener.
> > > 
> > Okay, so the question is:
> > If this._listener is null when _notifyServerReady is called, 
> > should we call |onServerReady| once the listener is set?
> 
> We can enforce that listener needs to be provided before doing
> startServer/close and document it in IDL. This assumption is already existed
> in all of our device provider implementation and I don't really want to
> change it.

Mm... How about |throw| an exception?
If it works for you, document it in IDL.
Comment on attachment 8776547 [details] [diff] [review]
part 2, support-tls-control-server.patch. v4

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

r+ with comment 33
Attachment #8776547 - Flags: review?(juhsu) → review+
(In reply to Junior [:junior] from comment #33)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #31)
> > (In reply to Junior [:junior] from comment #28)
> > > > @@ +125,5 @@
> > > > > +
> > > > > +  _notifyServerReady: function() {
> > > > > +    Services.tm.mainThread.dispatch(() => {
> > > > > +      if (this._listener) {
> > > > > +        this._listener.onServerReady(this._port, this.certFingerprint);
> > > > 
> > > > There is no guarantee that all the tasks in event queue will not unset
> > > > listener.
> > > > 
> > > Okay, so the question is:
> > > If this._listener is null when _notifyServerReady is called, 
> > > should we call |onServerReady| once the listener is set?
> > 
> > We can enforce that listener needs to be provided before doing
> > startServer/close and document it in IDL. This assumption is already existed
> > in all of our device provider implementation and I don't really want to
> > change it.
> 
> Mm... How about |throw| an exception?
> If it works for you, document it in IDL.

It doesn't seem necessary to throw an exception that no one can catch it and doesn't take any effect. :(
Comment on attachment 8776463 [details] [diff] [review]
part 3, retry-start-server-after-timeout.patch

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

r+ with comment 29
Attachment #8776463 - Flags: review?(juhsu) → review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #35)
> (In reply to Junior [:junior] from comment #33)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #31)
> > > (In reply to Junior [:junior] from comment #28)
> > > > > @@ +125,5 @@
> > > > > > +
> > > > > > +  _notifyServerReady: function() {
> > > > > > +    Services.tm.mainThread.dispatch(() => {
> > > > > > +      if (this._listener) {
> > > > > > +        this._listener.onServerReady(this._port, this.certFingerprint);
> > > > > 
> > > > > There is no guarantee that all the tasks in event queue will not unset
> > > > > listener.
> > > > > 
> > > > Okay, so the question is:
> > > > If this._listener is null when _notifyServerReady is called, 
> > > > should we call |onServerReady| once the listener is set?
> > > 
> > > We can enforce that listener needs to be provided before doing
> > > startServer/close and document it in IDL. This assumption is already existed
> > > in all of our device provider implementation and I don't really want to
> > > change it.
> > 
> > Mm... How about |throw| an exception?
> > If it works for you, document it in IDL.
> 
> It doesn't seem necessary to throw an exception that no one can catch it and
> doesn't take any effect. :(

Okay let's just document the necessity of listener
Attached patch part 1, check-server-tls-support.patch (obsolete) (deleted) — — Splinter Review 
rebase to latest m-c, carry r+.
Attachment #8775434 - Attachment is obsolete: true
Attachment #8776885 - Flags: review+
Attached patch part 2, support-tls-control-server.patch. v5 (obsolete) (deleted) — — Splinter Review 
update according to review comments and rebase to latest m-c, carry r+.
Attachment #8776547 - Attachment is obsolete: true
Attachment #8776886 - Flags: review+
Attached patch part 3, retry-start-server-after-timeout.patch (obsolete) (deleted) — — Splinter Review 
update according to review comments and rebase to latest m-c, carry r+.
Attachment #8776463 - Attachment is obsolete: true
Attachment #8776887 - Flags: review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #41)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6320bfd87555

found "dom/presentation/tests/xpcshell/test_tcp_control_channel.js | Test timed out" on Android 4.2 x86. will fix this before landing.
rebase to latest m-c, carry r+.
Attachment #8776885 - Attachment is obsolete: true
Attachment #8779144 - Flags: review+
fix android x86 test failure and rebase to latest m-c, carry r+.
Attachment #8776886 - Attachment is obsolete: true
Attachment #8779145 - Flags: review+
rebase to latest m-c, carry r+.
Attachment #8776887 - Attachment is obsolete: true
Attachment #8779146 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10159ae9f499
Part 1, establish TLS socket to encrypted control server. r=junior.
https://hg.mozilla.org/integration/mozilla-inbound/rev/99869310b0ec
Part 2, support TLS control server. r=junior.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c25ff855651a
Part 3, retry startServer after timeout. r=junior.
Keywords: checkin-needed
Attached file pull request for tv 2.6 (deleted) — 
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: low security while using 2-UA presentation device
Testing completed: yes with xpcshell test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a
Attachment #8779659 - Flags: approval-mozilla-b2g48?
Comment on attachment 8779659 [details]
pull request for tv 2.6

Approve for 2.6
Attachment #8779659 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8779659 [details]
pull request for tv 2.6

ni? @xeonchen for uplift
Flags: needinfo?(xeonchen)
Flags: sec-review?(ptheriault)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: