Masatoshi Kimura [:emk] Assignee
	Description • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/74b8493b5610 added the errorCode field to the nsITransportSecurity interface, but it didn't change the serialization code.
The SSL error reporting will not obtain correct error codes without this change.

I also fixed a bug of serializing errorMessage. This bug was not uncovered because errorCode was always zero in the e10 child before this change.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 1 • 10 years ago

Comment on attachment 8547526 [details] [diff] [review]
patch

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

Looks good, but needs some changes. r- for now.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +322,5 @@
>    rv = stream->Write32(mSubRequestsNoSecurity);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  rv = stream->Write32(mErrorCode);

Let's be explicit about this cast: static_cast<uint32_t>(mErrorCode)
(bug 711408 should fix this in a better way, but we should do this for now).

@@ +327,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  // Do not call formatErrorMessage if mErrorMessageCached is already set
> +  // because formatErrorMessage will fail in an e10s child process, 

nit: trailing whitespace (also, end the comment with a '.' instead of ',')
Also, does this just fail in e10s, or is it an assertion failure? If the latter, we should check if we're in the child process and return NS_ERROR_FAILURE if so.

@@ +408,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  // PRErrorCode will be a negative value
> +  mErrorCode = errorCode;

We should be explicit about this cast: static_cast<PRErrorCode>(errorCode)

	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 10 years ago

Resolved review comments.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> Also, does this just fail in e10s, or is it an assertion failure? If the
> latter, we should check if we're in the child process and return
> NS_ERROR_FAILURE if so.

Good point. I added an error check in formatMessage.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 3 • 10 years ago

Comment on attachment 8548173 [details] [diff] [review]
patch v2

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

Great - r=me with comments addressed. Although, it would be good to test this - can you add a test somewhere that uses add_connection_test and then serializes and deserializes the nsITransportSecurityInfo in the aWithSecurityInfo callback? (i.e. it should check that errorCode survives the process)

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +239,5 @@
>      return NS_OK;
>    }
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    result.Truncate();

We should just hoist this Truncate to the top of the function and remove it from the if (errorCode == 0) block.

@@ +331,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  // Do not call formatErrorMessage if mErrorMessageCached is already set
> +  // because formatErrorMessage will fail in an e10s child process.

Actually, I don't think this comment is necessary.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 4 • 10 years ago

Actually, it would be more useful to check that the error code is serialized/deserialized correctly in the SSL error reporting test. Is that test enabled for e10s?

	Masatoshi Kimura [:emk] Assignee
	Comment 5 • 10 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Actually, it would be more useful to check that the error code is
> serialized/deserialized correctly in the SSL error reporting test. Is that
> test enabled for e10s?

Currently the error code will not survive even in non-e10s mode because the SSL error reporting will explicitly serialize/deserialize TranportSecurityInfo on sending the event regardless whether the e10s is enabled [1][2].
Looks like browser_bug846489.js did not test for the error code at all [3]. I'll add a test for it.

[1] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/content.js#l268
[2] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/browser.js#l2641
[3] https://hg.mozilla.org/mozilla-central/rev/287ddaf9b9df

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 8 • 10 years ago

Comment on attachment 8548987 [details] [diff] [review]
Unit test

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

Great - r=me.

::: security/manager/ssl/tests/unit/test_cert_chains.js
@@ +50,5 @@
> +  let deserialized = serHelper.deserializeObject(serialized);
> +  deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> +  do_check_eq(securityInfo.securityState, deserialized.securityState);
> +  do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> +  do_check_eq(securityInfo.errorCode, deserialized.errorCode);

Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.

	:Felipe Gomes (needinfo for replies!)
	Comment 9 • 10 years ago

Comment on attachment 8548990 [details] [diff] [review]
mochitest_browser

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

I'm just curious why is this necessary

	Masatoshi Kimura [:emk] Assignee
	Comment 10 • 10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=553a27466906

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/ssl/tests/unit/test_cert_chains.js
> @@ +50,5 @@
> > +  let deserialized = serHelper.deserializeObject(serialized);
> > +  deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> > +  do_check_eq(securityInfo.securityState, deserialized.securityState);
> > +  do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> > +  do_check_eq(securityInfo.errorCode, deserialized.errorCode);
> 
> Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.

I did not add this because:
1. It already tested do_chack_eq(securityInfo.errorCode, <nonzero>) and do_chack_eq(securityInfo.errorCode, deserialized.errorCode)
2. deserialized.errorCode can be zero if the connection succeeds.

Instead, I added expectedErrorCode parameter to the function, moved the do_chack_eq into test_security_info_serialization, and added a test for the successful connection case.
In general, we should avoid do_check_neq if we know what is the expected value.

	Masatoshi Kimura [:emk] Assignee
	Comment 11 • 10 years ago

(In reply to :Felipe Gomes from comment #9)
> I'm just curious why is this necessary

Because it would have caught this earlier and SSL error report will be less useful if the server will always see zero for all errors.

	Masatoshi Kimura [:emk] Assignee
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6da326356fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b98d0a4fa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b459cff425

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b6da326356fb
https://hg.mozilla.org/mozilla-central/rev/48b98d0a4fa4
https://hg.mozilla.org/mozilla-central/rev/50b459cff425