1344340 - "An unknown error has occurred (804b000c)" appears in the statusbar when loading some sites

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: unknown-error.png

Sorry in advance for not getting a good, specific component; please triage as-needed!

Build ID: Build identifier: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

STR:

1. Load http://www.wired.com (this happens on other sites, like wiki.mozilla.org)
2. Look at the statusbar on the bottom left (default)

Actual:

Seemingly in-between DNS lookup and the actual 1st connection to the website/resource, you see "An unknown error has occurred (hex)" in the statusbar

Expected:

Not really sure, to be honest

Comment 1

[YF (Yang)]

https://dxr.mozilla.org/mozilla-central/search?q=%22An+unknown+error+has+occurred%22

Comment 2

[YF (Yang)]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=08df0e07f0ba02a27e532b25a0d3415166a6bba1&tochange=fb3c9156f1b99c4b08e7c35b98ac27756c357687

fb3c9156f1b9	Patrick McManus — Bug 1340655 - remove h1 pipeline support r=mayhemer

Comment 3

[Masatoshi Kimura [:emk]]

0x804b000c is NS_ERROR_NOT_CONNECTED, by the way.

Comment 4

[Patrick McManus [:mcmanus]]

Thanks for the research, but I'm going to disagree with the regression analysis - my testing shows this was introduced with bug 1340164 - which makes a lot more sense based on what the patches changed.

using gecko-dev
git checkout a980860d80cca1a2418d46ee0be9f80a921e1e4e~1
this is the cset before 1340655 and I can still see the bug

git checkout 29f090a0d67b4108293a35f8afe4e6395df18209~1
this is the cset before 1340164 and I can't see the bug

(this is a bit confusing because a980860d80cca1a2418d46ee0be9f80a921e1e4e~1 is 29f090a0d67b4108293a35f8afe4e6395df18209 .. they were landed back to back by coincidence)

Comment 5

[Dragana Damjanovic [:dragana]]

The bug to blame is 1343600. I have added 2 new socket events: so 0x804b000c is NS_NET_STATUS_TLS_HANDSHAKE_STARTING ans 0x804b00d is NS_NET_STATUS_TLS_HANDSHAKE_ENDED.

I will update  the test shown.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1344340.patch

I am not sure about working. If you have a better suggestion I will change it.

Comment 7

[Masatoshi Kimura [:emk]]

Why did you use duplicated value with NS_ERROR_NOT_CONNECTED/NS_ERROR_CONNECTION_REFUSED?
https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/xpcom/base/ErrorList.h#207,210
https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/xpcom/base/ErrorList.h#337-338

Didn't it confuse the status UI?

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Why did you use duplicated value with
> NS_ERROR_NOT_CONNECTED/NS_ERROR_CONNECTION_REFUSED?
> https://dxr.mozilla.org/mozilla-central/rev/
> 8d026c60151005ad942e3d4389318fe28a0c8c54/xpcom/base/ErrorList.h#207,210
> https://dxr.mozilla.org/mozilla-central/rev/
> 8d026c60151005ad942e3d4389318fe28a0c8c54/xpcom/base/ErrorList.h#337-338
> 
> Didn't it confuse the status UI?

Status codes do not have anything to do with errors. All almost all our status codes are some errors as well:
NS_NET_STATUS_RESOLVING_HOST is equal to NS_BINDING_REDIRECTED,
NS_NET_STATUS_CONNECTED_TO is equal to NS_BINDING_RETARGETED....

This status code are only different phases of a transaction and not the final status/error of a transaction.
This is a bit confusing, but that is a different bug.

And here is a comment saying that:
https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/xpcom/base/ErrorList.h#326

Comment 9

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

I think the NS_NET_STATUS_ codes used to be associated to SUCCESS(code).
I don't remember when this changed, or why.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f98abacba4
add text for NS_NET_STATUS_TLS_HANDSHAKE_STARTING and NS_NET_STATUS_TLS_HANDSHAKE_ENDED tansport states. r=valentin

Comment 11

[Carsten Book [:Tomcat]]

landed with correct bug number now, seems the patch here has the wrong one (bug 1343600)

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Carsten Book [:Tomcat] from comment #11)
> landed with correct bug number now, seems the patch here has the wrong one
> (bug 1343600)

Thanks!!!

Sorry, the other is the bug that caused the regression.

Comment 13

[Dragana Damjanovic [:dragana]]

It looks like this have not made it into aurora 54 :(

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96f98abacba4

Comment 17

[Dragana Damjanovic [:dragana]]

Comment on attachment 8843881 [details] [diff] [review]
bug_1344340.patch

Approval Request Comment
[Feature/Bug causing the regression]:1343600
[User impact if declined]: At the bottom right corner we show info about state of a connection,e.g. "Connecting to..", "Sending to..". I have added 2 new ones, tls started and tls ended, but I forgot to add text for it. Therefore users see "An unknown error has occurred (804b000c)" 
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: It is easy to reproduce. I have verified it manually.
[List of other uplifts needed for the feature/fix]: None, this one just missed getting into 54 for a little
[Is the change risky?]: none
[Why is the change risky/not risky?]: This only adds 2 strings
[String changes made/needed]: adding 2 strings

Comment 18

[Francesco Lodolo [:flod]]

I wonder why we don't back that out from aurora instead, but I confess I don't fully understand the scope of bug 1343600 and if it needs to be available on 54.

If release drivers decide this needs to land in aurora, the sooner it happens the better.

Comment 19

[Dragana Damjanovic [:dragana]]

(In reply to Francesco Lodolo [:flod] from comment #18)
> I wonder why we don't back that out from aurora instead, but I confess I
> don't fully understand the scope of bug 1343600 and if it needs to be
> available on 54.
> 
> If release drivers decide this needs to land in aurora, the sooner it
> happens the better.

We can back it out. Bug 1343600 is a small change and it is no risk to back it out.
Bug 1343600 fixes resource timing (https://www.w3.org/TR/resource-timing/), i.e. TLS handshake should be part of connect startStart-connectEnd time but it is not accounted at all.

Comment 20

[Dragana Damjanovic [:dragana]]

The timing bug is there from beginning it is not a regression.

Comment 21

[Gerry Chang [:gchang]]

Hi :dragana, 
I prefer to backout bug 1343600, can you provie a patch to backout 1343600 from aurora?

Comment 22

[Dragana Damjanovic [:dragana]]

Attachment: bug_1344340_backup1343600_aurora.patch

Comment 23

[Dragana Damjanovic [:dragana]]

Attachment: bug_1344340_backup1343600_aurora.patch

I am not sure if a review is required. This is only a backup of 1343600.

Comment 24

[Patrick McManus [:mcmanus]]

gary, dragana - you should take this uplift in aurora. it's a trivial followon to a real bugfix in the first 2 days of aurora. It doesn't make sense to make our users wait 6 additional weeks for the timing fix.

Comment 25

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Comment on attachment 8844856 [details] [diff] [review]
bug_1344340_backup1343600_aurora.patch

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

typo in commit message backup -> backout

Comment 26

[Patrick McManus [:mcmanus]]

Comment on attachment 8844856 [details] [diff] [review]
bug_1344340_backup1343600_aurora.patch

owner privilege - we need to be be flexible enough to fix stuff like this in the first couple days of aurora.

Comment 27

[Dragana Damjanovic [:dragana]]

Comment on attachment 8843881 [details] [diff] [review]
bug_1344340.patch

Approval Request Comment
[Feature/Bug causing the regression]:1343600
[User impact if declined]: At the bottom right corner we show info about state of a connection,e.g. "Connecting to..", "Sending to..". I have added 2 new ones, tls started and tls ended, but I forgot to add text for it. Therefore users see "An unknown error has occurred (804b000c)" 
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: It is easy to reproduce. I have verified it manually.
[List of other uplifts needed for the feature/fix]: None, this one just missed getting into 54 for a little
[Is the change risky?]: none
[Why is the change risky/not risky?]: This only adds 2 strings
[String changes made/needed]: adding 2 strings

Comment 28

[Gerry Chang [:gchang]]

Hi :flod,
There are 2 new strings added in the aurora cycle. If we need to take this, are you ok with it?

Comment 29

[Francesco Lodolo [:flod]]

(In reply to Gerry Chang [:gchang] from comment #28)
> Hi :flod,
> There are 2 new strings added in the aurora cycle. If we need to take this,
> are you ok with it?

Your call at this point (see comment 18).

Comment 30

[Gerry Chang [:gchang]]

Comment on attachment 8843881 [details] [diff] [review]
bug_1344340.patch

Add strings to show the status at the bottom right corner. Aurora54+.

Comment 31

[Gerry Chang [:gchang]]

Track 54+ as regression.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/359c6746a4c6f40347a7c6a3813977c1744ab603

Comment 33

[Patrick McManus [:mcmanus]]

Thanks all.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Build identifier: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0