1469826 - "not secure" icon displayed for non-existent host (where connection attempted over https)

Status: NEW

(DevTools :: Netmonitor, enhancement, P3)

Description

[nawuvi]

Attachment: test case

the "not secure" icon is shown when linking to a non-existent host, even when the connection is being attempted over https. this is misleading as it implies data was transferred in plain text, whereas that's not the case. it would seem to make the most sense for neither the "secure" nor "not secure" icon be shown

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

Agree, this is misleading. The entire request should rather somehow indicate 'failure' state (using an icon/color etc.)

Honza

Comment 2

[Dhyey Thakore [:dhyey35]]

Can I get assigned to this bug ?

How about something like this: https://imgur.com/a/lomigme

We will remove the "not secure" icon and have "FAIL" displayed instead of status 200.

Do you have any suggestions ?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to dhyey35 from comment #2)

Can I get assigned to this bug ?

How about something like this: https://imgur.com/a/lomigme

We will remove the "not secure" icon and have "FAIL" displayed instead of status 200.

Do you have any suggestions ?

@Harald, this looks better than before, what do you think?

Honza

Comment 4

[:Harald Kirschner :digitarald]

Agreed, great first step.

Only nit to try a smaller FAIL font to make it fit within the fixed width of the status column. I tried here with 2 font variations: https://www.figma.com/file/Vs7SyS5ELzEbiVubHnLjpvnX/Failed-Request-Status?node-id=0%3A1

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please look at the suggestion in comment #4

Honza

Comment 6

[Dhyey Thakore [:dhyey35]]

Attachment: Bug 1469826 - Display appropriate status for failed network request

Show FAIL text in status column

Comment 7

[Dhyey Thakore [:dhyey35]]

@Honza I have sent you a message on slack related to this bug.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for helping with this bug!

It feels a bit hacky using transferredSize == 0 and !status props to identify failed requests. But, what's actually the difference between failed and 404? Could we fix this fox 404s?
Or perhaps we need better API from the platform?

Honza

Comment 9

[Dhyey Thakore [:dhyey35]]

Yeah I agree, I will investigate further on diff btwn failed and 404. I gave a look to the chromium source and it seems they are having their c++ code determine it and than use it in front-end to add error class in network panel. So maybe we need better API from the platform.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

:kershaw, what is the best way to identify failed requests? Are there any platform API we could use for that? What's actually the difference between 404 and failed request?

Honza

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #10)

:kershaw, what is the best way to identify failed requests? Are there any platform API we could use for that? What's actually the difference between 404 and failed request?

Honza

I am not sure I understand correctly the definition of a "failed" request.
But, I think the best way to identify a failed request is checking the status code from OnStopRequest [1] and see if the status is NS_OK or not. Actually, a request with 404 response is also a successful request (the status code is NS_OK from OnStopRequest). It's just the response code [2] is 404.

[1] https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/netwerk/base/nsIRequestObserver.idl#36
[2] https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/netwerk/protocol/http/nsIHttpChannel.idl#258

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@dhyey35: please look at the links above, does it help?

Honza

Comment 13

[Dhyey Thakore [:dhyey35]]

Hey Honza, that did help and I have updated the code in phab. What I am doing now is when onStopRequest is called and status isn't NS_OK I am emitting an event that updates the failed state in client. Checking manually on few urls makes it seem that this works.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Can you please share the page (online or as an attachment) you are using for testing?
Honza

Comment 15

[Dhyey Thakore [:dhyey35]]

I was using the test case file attached above: https://bug1469826.bmoattachments.org/attachment.cgi?id=8986431 ( screenshot of FAIL ) and a node project on my localhost.

I have found an online rest api demo service that can be used for testing

Testing links:

1. http://dummy.restapiexample.com/api/v1/employees - should show 200 as it is an actual API mentioned on the page
2. http://dummy.restapiexample.com/api/v1/employees-FAKE - should show 404 as server send's that
3. http://dummy.restapiexample-FAKE.com/api/v1/employees - should show FAIL as no host with that URL exists

The above links worked as expected on my local build.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

I am not seeing the status FAIL in the Console panel, see the attached screenshot. Can we display it there too?

Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am seeing some test failures when applying this patch and also the one from re-enable netmonitor devtools tests (Bug 1258809)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88282c87299e5a7d1d1539567d1be85b08da6b4&selectedJob=236345641

Honza

Comment 18

[Dhyey Thakore [:dhyey35]]

The tests were failing because I had added a new prop failed. Auto generated the new file and updated phab along with changes.

I need some guidance as how to call batchedMessageUpdates from webconsole-wrapper.js as when the packet for requestStatus comes in it's 5th and there is only one packet after it, which makes them 6, while the code mentions that only after 8 packets have been received it would send update the UI. Should I update UI immediately when requestStatus packet arrives ?

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to dhyey35 from comment #18)

The tests were failing because I had added a new prop failed. Auto generated the new file and updated phab along with changes.

I need some guidance as how to call batchedMessageUpdates from webconsole-wrapper.js as when the packet for requestStatus comes in it's 5th and there is only one packet after it, which makes them 6, while the code mentions that only after 8 packets have been received it would send update the UI. Should I update UI immediately when requestStatus packet arrives ?

Ah, yes, this is our old a bit hacky place. Nicolas, do you have any recommendation for what to do here?

Honza

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Dhyey Thakore [:dhyey35] from comment #18)

I need some guidance as how to call batchedMessageUpdates from webconsole-wrapper.js as when the packet for requestStatus comes in it's 5th and there is only one packet after it, which makes them 6, while the code mentions that only after 8 packets have been received it would send update the UI. Should I update UI immediately when requestStatus packet arrives ?

When we have a requestStatus of failed, are we expecting any other data? If not, we could call batchedMessagesUpdates directly

if (res.networkInfo.updates.length === expectedLength || isFailedRequest) {
  this.batchedMessageUpdates({ res, message });
}

If we are still waiting for some data, we could also check that we have all we need, for example by having an expectedFailedRequestLength or something similar.

Comment 21

[Dhyey Thakore [:dhyey35]]

Attachment: network-1469826.png

Thanks Nicolas.

Honza I have updated the code and also attached a screenshot. Can you please take a look at one FR: comment in NetworkEventMessage.js file. Also please let me know if you have any suggestions.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Nice! The FAIL badge looks very red though :)
Could we use the same color than for the console error icon instead (var(--red-40))?

Comment 23

[Dhyey Thakore [:dhyey35]]

Attachment: network-1469826-1.png

I have changed the color to var(--red-40)

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the update!

A comment left in Phabricator.

Honza

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am seeing test failures on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c94a4b27930d30cf55456f008358cf833d4f7480

@Nicolas: it looks like we need to update the stubs (and include changes in the patch), correct?
mach test devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_update_stubs_network_event.js

Honza

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

yes, mach test devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_update_stubs_network_event.js --headless will update the stubs, and you can then run mach test devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_network_event.js --headless --verify to see if everything is okay.

Comment 27

[Dhyey Thakore [:dhyey35]]

@Honza I am not able to see the inline comment in phab

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Dhyey Thakore [:dhyey35] from comment #27)

@Honza I am not able to see the inline comment in phab

Should be there now, sorry.

Honza

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please see my comment in Phab (there are some test failures)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62a0789b40e7e82ed1397e512d3fbf6d74d78245&selectedJob=247745140

Honza

Comment 30

[Dhyey Thakore [:dhyey35]]

According to your comment in phab I created a new component and updated the code. You also mentioned using it in URL column, I couldn't find it. Can you pls help me with that ?

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Dhyey Thakore [:dhyey35] from comment #30)

According to your comment in phab I created a new component and updated the code. You also mentioned using it in URL column, I couldn't find it. Can you pls help me with that ?

Here is the RequestListColumnUrl component file:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListColumnUrl.js#20

It's relatively new, so you might need to get the latest m-c

Honza

Comment 32

[Patrick Brosset <:pbro>]

Hi Dhyey, I just noticed that this bug and the attached phabricator patch haven't been updated in a few months.
Do you still plan on working on this? If so and you just need more time, that's fine! If you, however, are not going to work on this anymore, please let us know so this bug can be made available for others to pick up.

Comment 33

[Dhyey Thakore [:dhyey35]]

Hi Patrick, A bit busy with inline preview in the Debugger tab, will take this up in a couple of weeks once it gets over.

Comment 34

[Patrick Brosset <:pbro>]

Unassigning for now, for lack of activity over a long period of time. Feel free to re-claim the bug if you plan on picking up the work. In the meantime, someone else might be interested in picking up where this was left off.