Closed Bug 870308 Opened 6 years ago Closed 6 years ago

GCLI appcache list command exception when cache disabled

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: miker, Assigned: miker)

Details

Attachments

(1 file, 4 obsolete files)

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
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
I have also removed the appCacheManifestContainsErrors string because it was a message that is never triggered.
Attachment #747392 - Flags: review?(jwalker)
Attached patch Patch v2 (obsolete) — Splinter Review
Added a comment
Attachment #747392 - Attachment is obsolete: true
Attachment #747392 - Flags: review?(jwalker)
Attachment #747393 - Flags: review?(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.
Attachment #747393 - Flags: review?(jwalker)
We could do that ... AppCacheUtils.jsm should be usable from other extensions though. I guess throwing still makes sense there.
Attached patch patch v3 (obsolete) — Splinter Review
Throw errors instead of returning strings.
Attachment #747393 - Attachment is obsolete: true
Attachment #747482 - Flags: review?(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?
Attachment #747482 - Flags: review?(jwalker)
Attached patch Patch v4 (obsolete) — Splinter Review
(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.
Attachment #747482 - Attachment is obsolete: true
Attachment #747876 - Flags: review?(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
Attachment #747876 - Flags: review?(jwalker) → review+
Attached patch Patch v32767Splinter Review
(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.
Attachment #747876 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e9eda6137518
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.