870308 - GCLI appcache list command exception when cache disabled

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

1. Go to about:config
2. Set browser.cache.memory.enable to false
3. Set browser.cache.disk.enable to false
4. Open GCLI
5. appcache list

Result:
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICacheService.visitEntries]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource:///modules/devtools/AppCacheUtils.jsm :: ACU_show :: line 251" data: no]

console.error: 
Object
  - prototype Object
    - QueryInterface = function QueryInterface() { [native code] }
    - columnNumber = 0
    - data = null
    - filename = resource:///modules/devtools/AppCacheUtils.jsm
    - initialize = function initialize() { [native code] }
    - inner = null
    - lineNumber = 251
    - location = {"language":2,"languageName":"JavaScript","filena_
    - message = Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICacheS_
    - name = NS_ERROR_NOT_AVAILABLE
    - result = 2147746065
  - prototype Object

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

I have also removed the appCacheManifestContainsErrors string because it was a message that is never triggered.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v2

Added a comment

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 747393 [details] [diff] [review]
Patch v2

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2163,2 @@
>          return context.createView({
> +          html: "<span>" + entries + "</span>"

Ug.
Think of these things that we are converting as types. We shouldn't be mixing errors into real things. The point of this is that we can push data from one command to another without resorting to output parsing. So starting off with "if (typeof entries === "string")" isn't right.

Instead just "throw new Error(msg)" in the command and we'll know it's an error.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We could do that ... AppCacheUtils.jsm should be usable from other extensions though. I guess throwing still makes sense there.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: patch v3

Throw errors instead of returning strings.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 747482 [details] [diff] [review]
patch v3

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2158,5 @@
>      from: "appcacheentries",
>      to: "view",
>      exec: function(entries, context) {
> +      if (typeof entries === "string") {
> +        // Error message returned

We can get rid of this can't we?

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v4

(In reply to Joe Walker [:jwalker] from comment #6)
> Comment on attachment 747482 [details] [diff] [review]
> patch v3
> 
> Review of attachment 747482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/BuiltinCommands.jsm
> @@ +2158,5 @@
> >      from: "appcacheentries",
> >      to: "view",
> >      exec: function(entries, context) {
> > +      if (typeof entries === "string") {
> > +        // Error message returned
> 
> We can get rid of this can't we?

What the heck is wrong with me? Of course we can, done.

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 747876 [details] [diff] [review]
Patch v4

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

r+ even if you decide my nit is too picky.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2164,5 @@
>          "    <table class='gcli-appcache-detail'>" +
>          "      <tr>" +
>          "        <td>" + gcli.lookup("appCacheListKey") + "</td>" +
>          "        <td>${entry.key}</td>" +
>          "      </tr>" +

Nit: I think it would be good to be in the habit of defining these outside of the converter for 2 reasons:
* It prevents us from string-concatenating user data, which is probably a security risk
* It's a touch closer to what we might like, which is for the HTML to be in a .html file

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v32767

(In reply to Joe Walker [:jwalker] from comment #8)
> Comment on attachment 747876 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 747876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ even if you decide my nit is too picky.
> 
> ::: browser/devtools/commandline/BuiltinCommands.jsm
> @@ +2164,5 @@
> >          "    <table class='gcli-appcache-detail'>" +
> >          "      <tr>" +
> >          "        <td>" + gcli.lookup("appCacheListKey") + "</td>" +
> >          "        <td>${entry.key}</td>" +
> >          "      </tr>" +
> 
> Nit: I think it would be good to be in the habit of defining these outside
> of the converter for 2 reasons:
> * It prevents us from string-concatenating user data, which is probably a
> security risk
> * It's a touch closer to what we might like, which is for the HTML to be in
> a .html file

Agreed and done.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/e9eda6137518

Comment 11

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/e9eda6137518