Closed Bug 1003860 Opened 10 years ago Closed 8 years ago

Support DOM Cache with storage inspector

Categories

(DevTools :: Storage Inspector, defect)

Product:
DevTools
Component:
Storage Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: overholt, Assigned: ochameau)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 5 obsolete files)

Various tweaks - v1
4.14 KB, patch
miker
: review+
Details | Diff | Splinter Review
patch v4
10.12 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Simplify setup tasks - v3
13.76 KB, patch
ochameau
: review+
Details | Diff | Splinter Review 
Much like winter, Service Workers are coming and with them, their Cache API:

https://github.com/slightlyoff/ServiceWorker/blob/master/caching.md

Alongside IDB and other storage inspection, we should support the Service Worker Cache.
(In reply to Andrew Overholt [:overholt] from comment #0)
> Much like winter, Service Workers are coming and with them, their Cache API:

"Much like winter" ^.^ XD
mass component move . filter on #MassComponentMoveStorageInspector
Component: Developer Tools → Developer Tools: Storage Inspector
I'm hopeful this can be implemented using the existing C++ API in CacheStorage.h and Cache.h.  The main thing you need is an nsIPrincipal for the page whose Cache you want to inspect.

Optimizer, does this sound like something you have available in devtools?
Flags: needinfo?(scrapmachines)
(In reply to Ben Kelly [:bkelly] from comment #3)
> I'm hopeful this can be implemented using the existing C++ API in
> CacheStorage.h and Cache.h.  The main thing you need is an nsIPrincipal for
> the page whose Cache you want to inspect.
> 
> Optimizer, does this sound like something you have available in devtools?

Not at the moment but I am sure we can bake it in.

Also see bug 1133601.
Flags: needinfo?(scrapmachines)
Assignee: nobody → mratcliffe
(In reply to Ben Kelly [:bkelly] from comment #3)
> I'm hopeful this can be implemented using the existing C++ API in
> CacheStorage.h and Cache.h.  The main thing you need is an nsIPrincipal for
> the page whose Cache you want to inspect.

Yes we can. Vivien crafted me this hacky patch in order to retrieve the `caches` object for a given principal.
That would be great if you could provide a less hacky patch, actually landable or/and sync-up with vivien:
  https://github.com/ochameau/mozilla-central/commit/f4e9665ae456f2771b1080224f30ae853a3e9bb1

Here is a small snippet to use it:
  Cu.import("resource://gre/modules/Services.jsm");

  var url = 'http://gaiamobile.org/fm/app/views/main/index.html';
  var uri = Services.io.newURI(url, null, null);
  var principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);

  var win = Services.appShell.hiddenDOMWindow;

  // XXX Don't use |caches| as a name of the variable if the code runs into a window,
  // as it is a read-only property of window and the next read will return the caches
  // object of the current window.

  // The second argument is a boolean indicating if you want to get |content| cache
  // or |chrome| cache.
  // The |content| cache is the cache explicitely named in the service worker, and
  // controlled by the app.
  // The |chrome| cache is the cache implicitely cached by the platform, hosting the
  // source file of the service worker.
  var c = win.getMozCaches(principal, true);

  // XXX Seems like there is a bug if we don't put that on the next tick.
  // The promise just return |false|. This is weird.
  setTimeout(function() {
    c.keys().then(function(names) { alert(names); });
  });

On top of that, to prove that we can actually use such approach I crafted this prototype within the storage actor:
  https://github.com/ochameau/mozilla-central/commit/59aac75743d3d2c31be203bc990bfb90c0d9ecd4
Its not that hacky.  I think the main thing I would do differently is just make it a [ChromeOnly] Constructor on CacheStorage.  Also, a webidl enumeration for the content-vs-chrome namespace.

So your example would change to:

  Cu.import("resource://gre/modules/Services.jsm");

  var url = 'http://gaiamobile.org/fm/app/views/main/index.html';
  var uri = Services.io.newURI(url, null, null);
  var principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);

  var c = new CacheStorage(principal, 'content');

This following does seem like a bug.  It may be something we are seeing in another test as well.  I'm not sure whats going on there yet, though.

  // XXX Seems like there is a bug if we don't put that on the next tick.
  // The promise just return |false|. This is weird.
  setTimeout(function() {
    c.keys().then(function(names) { alert(names); });
>   // XXX Seems like there is a bug if we don't put that on the next tick.
>   // The promise just return |false|. This is weird.

Does it resolve false or does it reject with an error?
See question in comment 7.
Flags: needinfo?(poirot.alex)
I don't know, that's something vivien reported. I just did what he suggested ;)
But I got his confirmation, yes, the promise resolves to false...

I didn't knew about Chrome Constructor feature, that sounds like a great option!
Flags: needinfo?(poirot.alex)
A way to explore the offline caches per domain was something discussed as desirable during the Service Workers work week in Madrid, adding the corresponding dependency. Thanks!
Blocks: ServiceWorkers-B2G
Status: NEW → ASSIGNED
Unassigning myself from this until I have more time.
Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
Blocks: sw-devtools
No longer blocks: ServiceWorkers-B2G
Cache is on Window, too.
Summary: Support Service Worker cache with storage inspector → Support DOM Cache with storage inspector
Attached patch patch v1 (obsolete) — — Splinter Review 
This patch now uses the chrome constructor, so no more hack involved.
Seems to work fine in e10s/non-e10s.
But there is an issue with cross domain requests.
As for the web content, we get access to an "opaque" Response object,
which doesn't let us know anything about the response.
So it doesn't allow us to tell what is the final status of the request.
We would need some platform magic for that.

Also, the actor doesn't tell us when there is new cache entry,
so you would have to reload the toolbox to see the changes.
I'm not sure Storage inspector already supports that?

Otherwise, storage inspector has a size limit which is quite painful
for Cache storage as you can easily have more than 50 entries.

I imagine we could have a first iteration with just the request URL,
and keep the response status for a next iteration, associated with a platform patch?
So that, we would mostly just need a test for this current patch.
We can add a way to get the inner response from chrome script.  That way all hidden values could be inspected regardless of response type.  Would that help?
(In reply to Ben Kelly [:bkelly] from comment #14)
> We can add a way to get the inner response from chrome script.  That way all
> hidden values could be inspected regardless of response type.  Would that
> help?

Yes. Exactly that.
Attached patch patch v2 (obsolete) — — Splinter Review 
Using :bkelly platform improvement to get response value.
With a mochitest.
If tree is green, this patch is ready for review.
Attachment #8705202 - Attachment is obsolete: true
Comment on attachment 8707962 [details] [diff] [review]
patch v2

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

::: devtools/server/actors/storage.js
@@ +997,5 @@
> +      let response = yield cache.match(request);
> +      // Unwrap the response to get access to all its properties if the
> +      // response happen to be 'opaque', when it is a Cross Origin Request.
> +      response = response.cloneUnfiltered();
> +      results.push(yield this.processEntry(request, response));

Note, the result of cloneUnfiltered() will always have Response.type === 'default'.  You probably want to track the original response type separately so it can be shown.
(In reply to Ben Kelly [:bkelly] from comment #18)
> Comment on attachment 8707962 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8707962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/storage.js
> @@ +997,5 @@
> > +      let response = yield cache.match(request);
> > +      // Unwrap the response to get access to all its properties if the
> > +      // response happen to be 'opaque', when it is a Cross Origin Request.
> > +      response = response.cloneUnfiltered();
> > +      results.push(yield this.processEntry(request, response));
> 
> Note, the result of cloneUnfiltered() will always have Response.type ===
> 'default'.  You probably want to track the original response type separately
> so it can be shown.

For now, I just display what Chrome displays: Request URL and Response Status-text.
We can surely show more information as we have it.
But it sounds like a good followup. Storage panel doesn't have a very flexible UI. So it may be more chanllenging to put more data on screen. Also I would need some help in what is good to be displayed as I'm not dogfooding much service workers/cache storage and don't really know what is really useful.
Assignee: nobody → poirot.alex
Attached patch Simplify setup tasks - v1 (obsolete) — — Splinter Review 
Same patch with multiple splits to help the review.

I don't know how this used to actually work, but that setup code
was really cryptic and it is hard to say it guarantees the indexedDB conditions.
createObjectStore *has* to be called within onupgradeended callback.

This patch makes it clear that we apply indexedDB conditions
and also stop doing these weirds `yield undefined;` to instead do
very basic Task.jsm code...

This helps me introducing new setup code for Cache storage in next patch!
Attachment #8709017 - Flags: review?(mratcliffe)
Attachment #8707962 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — — Splinter Review 
Main part of the patch. Introduces the actor bits.
I copied most logic from indexed db and then used the various tricks
:bkelly exposed (new CacheStorage() and cloneUnfiltered()) to fetch
the Cache storage data.

This patch also introduces a test for this new feature.
The data displayed for Cache storage is very basic.
It matches what Chromium already displays. Just the request URL
and the response status text.
We can followup to display more!

Also, in tests, I toggle 'dom.caches.testing.enabled' pref
to make it work as tests are loading html pages on http://
whereas Cache API is only available on https://.
Attachment #8709021 - Flags: review?(mratcliffe)
Random modifications that helped me working on this patch...
I'll comment them in individually.
Attachment #8709022 - Flags: review?(mratcliffe)
Comment on attachment 8709022 [details] [diff] [review]
Various tweaks - v1

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

::: devtools/client/storage/panel.js
@@ +59,5 @@
>        return this;
> +    }).catch(e => {
> +      console.log("error while opening storage panel", e);
> +      this.destroy();
> +    });

Today, if any exception happens in this code, it is completely ignored, not printed anywhere and the storage panel is destroyed and some other random stuff ends up breaking. Or, in my case, I was lucky that destroy was throwing and having a stack trace going back to this line!

With this patch, we immediately print the exception with the right stack trace about the original issue.

::: devtools/client/storage/test/head.js
@@ +482,5 @@
>    gUI.tree.expandAll();
>  
>    let selector = "[data-id='" + JSON.stringify(ids) + "'] > .tree-widget-item";
>    let target = gPanelWindow.document.querySelector(selector);
> +  ok(target, "tree item found with ids " + JSON.stringify(ids));

It may happen that the selector doesn't match anything and then we end up with random exception in click() method called some lines after.
Now, we can immediately know what is missing/wrong.

::: devtools/client/storage/ui.js
@@ +338,5 @@
> +      try {
> +        typeLabel = L10N.getStr("tree.labels." + type);
> +      } catch(e) {
> +        console.error("Unable to localize tree label type:" + type);
> +      }

These two try catch allow to start working on a new actor without providing a localization. Otherwise, if you miss any string in l10n files, it throws and prevent displaying anything.
Attachment #8709017 - Flags: review?(mratcliffe) → review+
Attachment #8709021 - Flags: review?(mratcliffe) → review+
Comment on attachment 8709022 [details] [diff] [review]
Various tweaks - v1

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

These little tweaks add some stability, especially when contributing to the codebase.

I really like this patch series because it shows how simple it is to add new storage types, thanks for working on this!
Attachment #8709022 - Flags: review?(mratcliffe) → review+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #25)
> I really like this patch series because it shows how simple it is to add new
> storage types, thanks for working on this!

Stole the words out of my mouth!
One last push to try, I'm unlucky these days with my checkins...
Attached patch Simplify setup tasks - v2 (obsolete) — — Splinter Review 
Fixed typo that broke dt7-browser_storage_values.js.
Attachment #8709519 - Flags: review+
Attachment #8709017 - Attachment is obsolete: true
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6741607&repo=fx-team
Flags: needinfo?(poirot.alex)
PGO failure, how fun is that!
I get this exception, in populateStoresForHost:
  TypeError: this.hostVsStores is null
This is weird as I copied indexedDB code for that, so it should also happen there, but it looks like it isn't. But I see that it may somehow happen if we get a window-ready event while the actor is still initializing.
Flags: needinfo?(poirot.alex)
Attached patch patch v4 — — Splinter Review 
Ok, so that was it. We just need to set `hostVsStores` earlier.

PGO try without the fix:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=798480345d52&selectedJob=15863139
PGO try with the fix:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4caa5cdff65&selectedJob=15817395
Attachment #8711623 - Flags: review+
Attachment #8709021 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/2ecd643f6c78d5a6af7c7e9b5dcb38b45ae02cbe
Backed out 3 changesets (bug 1003860) for dt6//browser_storage_basic.js failure on win7-debug
New failure, this time on window-debug, dt6 e10s - browser_storage_basic.js
  https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=fd5530566259
  https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=41046a32d9ff

devtools/client/storage/test/browser_storage_basic.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW
While rebasing, I got some merge conflict on some CPOW related fixes.
With some luck, it may have fixed these exceptions...
Some more test cleanup. I don't think it makes the tests turned green,
but I had some commented code in my previous patch.

Try looks green, I think a fix has been landed against head.js recently.
Attachment #8714704 - Flags: review+
Attachment #8709519 - Attachment is obsolete: true
docs -> https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cache_Storage

Please let me know if this covers it.
Flags: needinfo?(poirot.alex)
It says "any caches created by Service Workers" but DOM Caches can be created by non-{Service,} Workers.
Thanks for catching this, Andrew. I've replaced that with "DOM caches created using the Cache API". Does that sound OK?

I've also filed bug 1257671 :).
(In reply to Will Bamberg [:wbamberg] from comment #54)
> Thanks for catching this, Andrew. I've replaced that with "DOM caches
> created using the Cache API". Does that sound OK?

Yep!
(In reply to Will Bamberg [:wbamberg] from comment #52)
> docs ->
> https://developer.mozilla.org/en-US/docs/Tools/
> Storage_Inspector#Cache_Storage
> 
> Please let me know if this covers it.

Yes. Thanks!
Flags: needinfo?(poirot.alex)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: