1321820 - Storage Inspector fails with dom.caches.enabled=false

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P2)

Description

[wavexx]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20161201031821

Steps to reproduce:

Set dom.caches.enabled=false in the prefs.
Open the storage inspector. The "Caches" tree is now hidden, but the remaining storage options (local/session storage, cookies) do not work any longer.


Actual results:

The following is shown the browser console:

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Fri Dec 02 2016 16:45:17 GMT+0100 (CET)
Full Message: TypeError: CacheStorage is not a constructor
Full Stack: .getCachesForHost<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:1146:17
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
.populateStoresForHost<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:1222:24
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource


Expected results:

If dom.caches.enabled is kept as a pref, the storage inspector shouldn't fail if it is set to false.

Comment 1

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

To be clear, this pref disables the DOM cache (otherwise known as the service worker cache).

Comment 2

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

Attachment: Bug 1321820 - Storage Inspector fails with dom.caches.enabled=false

Because this bug is only caused when a major browser feature is disabled there is no need for a test in the same way that we don't try switching JavaScript off and using the debugger.

Review commit: https://reviewboard.mozilla.org/r/100806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100806/

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8821562 [details]
Bug 1321820 - Storage Inspector fails with dom.caches.enabled=false

https://reviewboard.mozilla.org/r/100806/#review101314

> Because this bug is only caused when a major browser feature is disabled there is no need for a test in the same way that we don't try switching JavaScript off and using the debugger.

I will argue that it would be nice to have a test to make sure the storage inspector is really not "broken" when dom.caches.enabled is set to false. What do you think ?
It would be even better to display some kind of message in the panel to tell the user why there isn't anything in this panel ("Since you set dom.caches.enabled to false, there isn't anything to see here", or something like that. It might be a good thing to do as a follow-up.

Comment 4

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

There is no hurry... I will add a test and see about adding some kind of messaging system.

Comment 5

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8821562 [details]
> Bug 1321820 - Storage Inspector fails with dom.caches.enabled=false
> 
> https://reviewboard.mozilla.org/r/100806/#review101314
> 
> > Because this bug is only caused when a major browser feature is disabled there is no need for a test in the same way that we don't try switching JavaScript off and using the debugger.
> 
> I will argue that it would be nice to have a test to make sure the storage
> inspector is really not "broken" when dom.caches.enabled is set to false.
> What do you think ?
> It would be even better to display some kind of message in the panel to tell
> the user why there isn't anything in this panel ("Since you set
> dom.caches.enabled to false, there isn't anything to see here", or something
> like that. It might be a good thing to do as a follow-up.

If we decide to act on all of the Firefox prefs we are entering a very large rabbit hole.

I think in this instance you are right though... I have created bug 1325650 to display a message in each storage panel's table when that storage type is preffed off.

I will still add a test with this patch though.

Comment 6

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

Comment on attachment 8821562 [details]
Bug 1321820 - Storage Inspector fails with dom.caches.enabled=false

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100806/diff/1-2/

Comment 7

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

Added test.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8821562 [details]
Bug 1321820 - Storage Inspector fails with dom.caches.enabled=false

https://reviewboard.mozilla.org/r/100806/#review101346

Looks good !
r+ assuming TRY is green

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63b00cc7babb
Storage Inspector fails with dom.caches.enabled=false r=nchevobbe

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63b00cc7babb