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.

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)

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.

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.

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?

(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

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.

Comment on attachment 8548990 [details] [diff] [review] mochitest_browser Review of attachment 8548990 [details] [diff] [review]: ----------------------------------------------------------------- I'm just curious why is this necessary

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.

(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.

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

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