exceptionDialog.js uses sync XHR request

ASSIGNED
Assigned to

Status

()

Core
Security: PSM
P3
normal
ASSIGNED
4 years ago
a year ago

People

(Reporter: Gijs, Assigned: Thomas Wisniewski)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [psm-cleanup])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
While bug 453855 added a timeout to make sure the dialog isn't held back by the cert check, I actually don't know why we don't make the cert check async rather than sync, rather than doing main-thread HTTP I/O which seems like a Bad Thing...
(Reporter)

Comment 1

4 years ago
FWIW, this is hg.mozilla.org/mozilla-central/annotate/e86b84998b18/security/manager/pki/resources/content/exceptionDialog.js#l133 and it causes an error to be shown in the browser console complaining about the sync XHR and why it's a bad idea.
If/when I or someone gets around to fixing bug 1035493, this will be taken care of.
See Also: → bug 1035493
(Reporter)

Updated

4 years ago
Depends on: 1035493
See Also: bug 1035493

Updated

4 years ago
Duplicate of this bug: 721651
Component: Security: UI → Security: PSM
Priority: -- → P3
Whiteboard: [psm-cleanup]
(Assignee)

Comment 5

2 years ago
Created attachment 8780873 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

I stumbled on this bug while hunting for XHR-related issues, and figured I'd contribute a patch (tested manually, since there don't seem to be any in-tree tests for this code). Feel free to take it, if it's still worth the effort.

Here's a try run, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27d5e1536ff
Attachment #8780873 - Flags: review?(dkeeler)
Comment on attachment 8780873 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

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

Thank you for the patch! This does the right thing, but I think there are a few improvements to be made that will both simplify the implementation and fix another few bugs at the same time (see below).
In terms of tests, I'm not aware of anything that would test this code path currently, but I think it would be not too difficult to adapt the code in https://dxr.mozilla.org/mozilla-central/rev/6e191a55c3d23e83e6a2e72e4e80c1dc21516493/browser/base/content/test/general/head.js#1112 to do what we want. Let me know if that's something you're interested in.

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +105,5 @@
>  
> +  function done() {
> +    gChecking = false;
> +
> +    if (req.channel && req.channel.securityInfo) {

If you use the event parameter that gets passed to this function, you can use event.target instead of req, which allows you to define this function in the top-level scope, which I think would be preferable to being inside another function (although in that case it would need a more descriptive name, like checkCertDone or something).

@@ +119,4 @@
>    var req = new XMLHttpRequest();
>    try {
>      if (uri) {
> +      req.onload = req.onerror = done;

I think it would be best to have one assignment per line here.

@@ +124,1 @@
>        req.channel.notificationCallbacks = new badCertListener();

We can actually get rid of badCertListener (see bug 1263765) by adding:

if (event.type == "error") {
  gBroken = true;
}

to the onload/onerror callback.

@@ +128,2 @@
>      }
>    } catch (e) {

Since we have the onerror handler, the try/catch shouldn't be necessary. The one remaining thing that would be throwing exceptions is getURI, which should really do its own try/catch if it gets a malformed URI (try putting "   " in the url field and clicking "Get Certificate" - this is another preexisting bug).
Attachment #8780873 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 7

2 years ago
Created attachment 8781612 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

Review comments addressed. It's mildly annoying to add more code that's dependent on xhr.channel, but I guess we're stuck with that UI for the foreseeable future anyway :)


>Let me know if [making tests for this is] something you're interested in.

I don't really mind, but it won't be a high priority for me. Would it be best to make a new bug for that, or just leave this one open with a note that tests are still needed?
Assignee: nobody → wisniewskit
Attachment #8780873 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8781612 - Flags: review?(dkeeler)
(Assignee)

Comment 8

2 years ago
I meant "mildly annoying to see req.channel code that we can't get rid of", and that we're stuck with that req.channel API. Guess my coffee tank is running a bit low...
Comment on attachment 8781612 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

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

Awesome. Just a few nits I forgot to mention last time. As for tests, this shouldn't land without at least basic coverage that this does what we expect it to. Usually we do the test work in the same bug as the implementation change.
What's the concern with using xhr.channel - is it deprecated? (Is there a better API we should be using?)

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +74,5 @@
>    gBroken = false;
>    updateCertStatus();
>  
> +  try {
> +    var uri = getURI();

nit: I forgot to mention it would be nice to update var to let where we're changing these lines anyway (which, in the case of uri, would require declaring it outside the scope of the try block)

@@ +76,5 @@
>  
> +  try {
> +    var uri = getURI();
> +  } catch(_) {
> +  } finally {

nit: I would just do without the finally here (makes the code a bit easier to read, I think).

@@ +78,5 @@
> +    var uri = getURI();
> +  } catch(_) {
> +  } finally {
> +    if (!uri) {
> +      checkCertDone({type: "error"});

Let's also define a "target" property with the value null.

@@ +86,5 @@
>  
>    var req = new XMLHttpRequest();
> +  req.onload = checkCertDone;
> +  req.onerror = checkCertDone;
> +  req.open('GET', uri.prePath);

nit: if you feel like it, you can update this to use double-quotes instead of single-quotes
Attachment #8781612 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8782071 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

Here's a new version with nits addressed.

>As for tests, this shouldn't land without at least basic coverage that this does what we expect it to. Usually we do the test work in the same bug as the implementation change.

OK, I'll try to find some time to make sure it's tested before I land it.

>What's the concern with using xhr.channel - is it deprecated? (Is there a better API we should be using?)

Oh, it's not really worth worrying about that (especially since addons still rely on xhr.channel). I'm just helping to refactor the XHR code and help it be as spec-compliant as possible, so I'm always itching to remove any non-standard behavior as I come across it :)
Attachment #8781612 - Attachment is obsolete: true

Comment 11

a year ago
In doing some work for cert exception handling in Tb, I noticed all the errors in the dialog and was going to file a bug. Fortunately this patch gets rid of the problem; hopefully it can make 52.
(Reporter)

Comment 12

a year ago
Comment on attachment 8782071 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

Looks like this got lost. Keeler, can you doublecheck and, if good to land, set r+ and mark checkin-needed? :-)
Attachment #8782071 - Flags: review?(dkeeler)
(Assignee)

Comment 13

a year ago
David mentioned in comment 9 that we shouldn't land this without tests. I was hoping to write some now soon, but if it's alright to land them separately then that's fine by me.
Comment on attachment 8782071 [details] [diff] [review]
1029456-use_async_XHRs_in_exceptionDialog.js.diff

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

This looks good. Again, though, I don't think the issue being addressed meets the requirements for checking in a change without tests.
Attachment #8782071 - Flags: review?(dkeeler) → review+
You need to log in before you can comment on or make changes to this bug.