245082 - Show Detailed Errors for Network/Parsing Failures during update checking

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

When update checking occurs, we should show detailed errors in the "Details"
dialog that the Update Wizard offers instead of offering text at the top that
describes all possible outcomes. We get the error status from the RDF XML
Datasource loader and the WSDL Loader so we can actually supply this info. I
suggest providing a second column to the list view that provides string
information about the error.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

See nsDocShell::DisplayLoadError for all of the network errors and the string
keys in docshell.properties that they map to

Comment 2

[Asa Dotzler [:asa]]

L10N impact so we need this before b4. 
Rob, can you look at this for us? Important for L10N and QA both. Thanks.

Comment 3

[Robert Strong (they/them - no direct email)]

Asa, I'll take a look at this and those other bugs in the next day or two. Cheers

Comment 4

[Robert Strong (they/them - no direct email)]

I've taken a similar approach to the one used by the new update service.
grab the data using nsIXMLHttpRequest which will give us the detailed errors
load the data if it is valid into nsIRDFDataSource using nsIRDFXMLParser
then off we go...

There are a couple of cases that need to be specially handled
UMO doesn't return anything when the installed version is the latest version
when the rdf is valid but doesn't contain data for the extension being checked.

I should have this done in a couple of days.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: screenshot

Screenshot of the error info. This was created by modifying the updateURL for
serveral extensions in the extensions.rdf to generate errors (e.g. invalid
updateURL, malformed update rdf, etc.). I should have a patch submitted this
evening.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch

Sorry, one line snuck into the last patch.

Benjamin - I kept the prior method as a fallback in case an app isn't built
with xmlhttprequest. They both get much better error reporting but the
xmlhttprequest case has several additional errors. Also, when update was split
pinstripe never got an update.css - this adds it.

Comment 8

[Robert Strong (they/them - no direct email)]

forgot to rev the uuid... I'll do so after the first review

Comment 9

[Benjamin Smedberg]

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

r=me

However, this is a very large change to make for 1.8: are there portions of
this patch that are needed for 1.8 (the styling changes, for instance), or can
we push this entire patch off to 1.9?

Comment 10

[Robert Strong (they/them - no direct email)]

All of the style changes are additions for the new ui element that displays the
errors except for one change to pinstripe's extensions.css where view was not
changed to richlistbox when the new widget landed and the adding of update.css
which was overlooked when bug 296566 added update.css to winstripe and qute.

There is also a ton of cleanup that should be done to extension update but I
didn't think it appropriate to include that in this bug.

As for pushing it off, I took the same approach as the new update system and
believe the risks to be the same except when it comes to the ui changes which
are easy to fix if they come up and I have already tested both pinstripe and qute.

Comment 11

[Robert Strong (they/them - no direct email)]

bsmedberg asked for an analysis of the risk/benefit ratio for this patch in
relation to 1.1

This patch:
1) changes the default method for reading the update rdf to using
xmlhttprequest. I used the same methodology for the most part as is used by the
new app update code and I believe the risk is the same or less than that code
since this will fallback to using the previous method if xmlhttprequest is not
available on the system. I have tested this with both Firefox and Thunderbird
and believe the risk is low. There is some complexity added here that isn't in
the app update code to handle the special case of not reporting an error for
extensions that are checked against UMO when an updateURL is not specified. The
benefit is that detailed errors are displayed in the update wizard ui when
performing an extension or theme update.

2) adds a ui element (e.g. richlistbox) to display the errors and associated
styling. The risks regarding using the element are IMO low and are also shared
elsewhere by what I believe are more important ui's as in the extension, theme,
and download managers. The styling of the element has been tested with both
winstripe and qute. The benefit is we have a ui for displaying the errors.

3) adds update.css to pinstripe which was overlooked when the update code was
moved to extensions by the partial landing of bug 296566. I believe the risk in
doing this is low and the risk in not doing this is high. I had to add it with
this patch since the update wizard needs it to style the added element mentioned
above. The benefit is the update wizard is styled as it was previously when
using the pinstripe theme.

4) Adds error strings including a course of action for the associated error
messages. There is the risk of suggesting a course of action that is incorrect
for a specific error. This can be mitigated by removing the suggested course of
action or by tuning the suggestions prior to 1.1 if it is necessary to do so. I
believe this is somewhat risky but it can be mitigated easily by updating the
strings. The benefit is this brings value to the error messages by giving the
user a course of action to resolve the error.

5) adds code to allow copying the error to the clipboard using the keyboard
(e.g. en-US accel+C). This makes it easier for a user to inform anyone they are
notifying of an error what the exact error that was displayed is. I believe the
risk is low but the benefit would be greatly improved by this having a ui
element. This provides a method for clearer communication when reporting errors
(e.g. this is the error instead of it just doesn't work, etc.). This can be
removed though I think the risk is low in relation to the potential benefit.

6) Cleaned up a small text glitch where the extension name during an update was
followed by " ..." (e.g. an extra space when compared to all other text followed
by a "...") instead of "...". Risk low but this can be removed without any
issues if desired.

7) fixes one oversight in pinstripe's extensions.css when the rchlistbox widget
was landed (e.g. view should have been changed to richlistbox). The risk is IMO
low in doing this and high in not doing this.

Notes: this patch brings update very close to being able to inform the user
properly when an app compatibility version update has been done. Currently when
this happens the user is informed that no updates are found even though
extensions that were not compatibile and have been made compatible will be
re-enabled on restart.

Comment 12

[Robert Strong (they/them - no direct email)]

Since this probably won't land in time for 1.1a2 I opened bug 299193 and bug
299196 to fix the two theme regressions as noted in items 3 and 7.

Comment 13

[Robert Strong (they/them - no direct email)]

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

This patch will bit rot shortly and I'm not comfortable landing this patch now
that there is the possibility of not having a day to fix any possible
regressions. Just comment in this bug or email me that it can be landed and
I'll submit an updated patch.

Comment 14

[Asa Dotzler [:asa]]

Robert, does this still apply in the new Update world? 

Comment 15

[Robert Strong (they/them - no direct email)]

Asa, it is still applicable though the patch on this bug is bit rotted. I will
give the code a once over tonight and submit an updated patch.

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: patch

I removed the copy code as Benjamin asked, changed the code so it doesn't
change the interface, and removed the majority of little fixes that aren't part
of this bug. I'm going to give this one more once over before asking for
review.

Comment 17

[Robert Strong (they/them - no direct email)]

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

Benjamin - I simplified the patch quite a bit. One thing this patch doesn't do
is clean up the unused items most of which are there due to extension update
being split off from update.

Comment 18

[Benjamin Smedberg]

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

I don't really think we want to do this: it looks like the combination of
no-cache headers and subsequent GetDataSource will fetch the RDF twice.

I'm feeling queasy about this code in general: it looks to me like we should
have some sort of more watchable RDF loading interface.

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to comment #18)
> (From update of attachment 189231 [details] [diff] [review] [edit])
> I don't really think we want to do this: it looks like the combination of
> no-cache headers and subsequent GetDataSource will fetch the RDF twice.
It uses XMLHttpRequest when it is available and then early returns before
getting to the GetDataSource and I left GetDataSource as a fallback method in
case there is an app that doesn't supply XMLHttpRequest. I am fine with removing
it as a fallback if all apps will supply XMLHttpRequest but this should be safe
because of the early return.

Comment 20

[Darin Fisher]

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

deferring to ben instead as he knows this code much better than me.

Comment 21

[Robert Strong (they/them - no direct email)]

The patch bit rotted and no longer makes sense with the changes to the update ui
(e.g. it is now integrated into the EM). Now there is no place to provide
"detailed" errors... at best we could provide short and vague errors.

I don't see this making it into 1.8b4 or 1.5 and if it was too risky for 1.8b3
when the first patch was initially submitted then it is even more so for 1.8b4.
I think it would be best to revisit this as part of the EM ui update.
http://wiki.mozilla.org/Firefox:Extension_Manager_UI

Comment 22

[Asa Dotzler [:asa]]

too late for this kind of change in 1.8b4/1.5

Comment 23

[Robert Strong (they/them - no direct email)]

For what it is worth bsmedberg r+'d the large patch that this was ported from
and was considered too large of a change to land for 1.5

Comment 24

[Robert Strong (they/them - no direct email)]

ignore that... it was meant for bug 307235 :(

Comment 25

[Robert Strong (they/them - no direct email)]

I personally don't think additional details need to be provided in the ui for this as it is implemented today and additional details are provided in the JavaScript console. I think it would just add clutter for something that would seldom be used. Reassigning to default and IMO this is wontfix since we do provide enough detail for the majority of users as is.

Mike and Ben, what do you think?

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, that's right, Rob; adding clutter for error messages that aren't meaningful to the majority of users doesn't add value here. 

The only thing I might suggest is that if/when bug 76111 lands, we pop some sort of warning or message telling users that they aren't connected to the network and so they can't check for updates (or just disable the button), but that feels like a different bug to me.