1175850 - Enable storage inspector tests after upgrading for E10S

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect)

Comment 1

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

Attachment: 1175850-storage-inspector-enable-tests.patch [WIP]

E10S still disabled... seems to be something to do with the message manager.

Comment 2

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

Also re-enable the storage inspector for browser/devtools/framework/test/browser_toolbox_window_shortcuts.js

Comment 3

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

Also:
browser/devtools/framework/test/browser_toolbox_tool_ready.js
browser/devtools/framework/test/browser_toolbox_tool_remote_reopen.js

Due to docshell leaks.

Comment 4

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

Attachment: 1175850-storage-inspector-enable-tests.patch

I'll hold off asking for review until try comes back but locally everything seems just peachy.

Comment 5

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

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74cd49589b2

Seems like we have a crasher.

Comment 6

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

Ignoring the crasher for now as there are some reproduceable failures in the actor tests.

Easy enough to reproduce:
./mach mochitest toolkit/devtools/server/tests/browser/ --start-at=toolkit/devtools/server/tests/browser/browser_storage_dynamic_windows.js --end-at=toolkit/devtools/server/tests/browser/browser_storage_updates.js

Comment 7

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

Attachment: 1175850-storage-inspector-enable-tests.patch

This patch:
1. Re-enables the following tests that were failing with the current implementation:
  - browser_toolbox_tool_ready.js
  - browser_toolbox_tool_remote_reopen.js
  - browser_toolbox_window_shortcuts.js
2. Properly handles the storage panel failing to open.
3. Cleans up numerous leaks via better cleanup in destructors.
4. Enables the following tests in E10S mode:
  - browser/devtools/storage/test/*
5. Adds the proper debugging helpers to browser/devtools/storage/test/head.js
6. The storage inspector polls every 500ms and updates data in the tables if anything needs to be done each time. This has been replaced by a proper batching system (no more polling).
7. We no longer attempt to remove message listeners from the destructor. We now do this when the message manager has been disconnected.
8. We no longer throw on ERR_DIRECTOR_CHILD_NO_REPLY or ERR_DIRECTOR_CHILD_MULTIPLE_REPLIES as this causes the browser to leak on shutdown.
9. Various fixes to message manager handling code.
10. Enables all storage tests on E10S builds.
11. Various test fixes although bugs have been logged to further improve these tests.

I will hold off asking for review until I have a green try.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12e994793183

Comment 8

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

Pulled from a greenish revision as I am not convinced that these failures are due to this patch.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4818b937d5b6

Comment 9

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

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89653f5161bc

Comment 10

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

Finally green... I will tidy the code up a little more ready for review.

Comment 11

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

Attachment: Patch 1: 1175850-storage-inspector-eslint-fixes.patch

Sorry for the delay... had a leak after my latest pull.

ESLint Fixes.

Comment 12

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

Attachment: Patch 2: 1175850-storage-inspector-enable-tests.patch

I'll hold off on review until we have a green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb58eff9506

Comment 13

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

Comment on attachment 8648019 [details] [diff] [review]
Patch 2: 1175850-storage-inspector-enable-tests.patch

On second thoughts I already expect a green try.

Comment 14

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

pbrosset: The test html files have been mostly left because indexedDB needs very special treatment in browser tests (it is handled the same way in all browser chrome tests).

A Stack Overflow question covering the special code is here although you don't really need to understand that code for this review:
http://stackoverflow.com/questions/10924624/explain-how-a-generator-is-used-in-this-javascript-code-with-indexeddb

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8648019 [details] [diff] [review]
Patch 2: 1175850-storage-inspector-enable-tests.patch

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

Thanks Mike, this is a lot of work, and not easy work. Thanks for spending time on it.
I didn't see anything major here, but I have some remarks and questions below, that I'd like answered before I R+

::: browser/devtools/storage/test/browser_storage_sidebar.js
@@ +100,5 @@
> +add_task(function*() {
> +  yield openTabAndSetupStorage(MAIN_DOMAIN + "storage-listings.html");
> +
> +  for (let test of testCases) {
> +    let { location, sidebarHidden, sendEscape} = test;

nit: either no spaces after { or a space before } too.

::: browser/devtools/storage/test/head.js
@@ +80,5 @@
>   * @param url {String} The url to be opened in the new tab
>   *
>   * @return {Promise} A promise that resolves after storage inspector is ready
>   */
> +function* openTabAndSetupStorage(url) {

You made this a generator function while before it was a normal function that returned a promise.
I think it's worth saying in the comment that it's a generator, it's easy to miss the * sign and call it with yield.

@@ +198,5 @@
>    Cu.forceCC();
>    Cu.forceShrinkingGC();
>  }
>  
> +function getAllWindows(baseWindow=gWindow) {

nit: add some jsdoc here please.

@@ +484,5 @@
>    gUI.tree.expandAll();
> +
> +  let animations = gUI.tree._parent.getAnimations();
> +  for (let animation of animations) {
> +    animation.finish();

A couple of questions about this:
- are you sure this is fully implemented in the WebAnimations API? For some reason I'm getting errors when trying to call finish() on some simple animation test page. But I haven't ran this test yet.
- Doesn't finish return a promise? I know several methods in the API do, and this one might, but I haven't checked.

@@ +504,5 @@
>   *        The id of the row in the table widget
>   */
> +function* selectTableItem(id) {
> +  let target = gPanelWindow.document
> +                 .querySelector(".table-widget-cell[data-id='" + id + "']");

nit: please align .querySelector with gPanelWindow on the line before.
Or maybe make a variable for the selector so this line is shorter.

::: browser/devtools/storage/test/storage-complex-values.html
@@ +82,5 @@
>    console.log("added cookies and stuff from main page");
>    callback();
> +};
> +
> +function* deleteDB(dbName) {

If the function returns a promise, and is called with 'yield deleteDB(...)' within a Task, then there's no need to make that function a generator with the * character. It doesn't itself use yield.

::: browser/devtools/storage/test/storage-listings.html
@@ +77,4 @@
>    callback();
> +};
> +
> +function* deleteDB(dbName) {

Same remark as in the previous file.

::: browser/devtools/storage/test/storage-secured-iframe.html
@@ +54,4 @@
>    callback();
> +};
> +
> +function* deleteDB(dbName) {

And here.

::: browser/devtools/storage/ui.js
@@ +84,5 @@
> +    // The debugger server adds .from to packets for tracking purposes. Because
> +    // we need to iterate through this object we simply delete .from as the
> +    // performance hit when iterating over the object four times is
> +    // inconsequential.
> +    delete storageTypes.from;

After reading the comment above this line and after quickly going through the code, it's still not clear to me why you're deleting the .from property, so I suspect future readers of this code will ask themselves the same question.
Wouldn't it be clearer if you just skipped the .from inside the for loop inside populateStorageTree?

::: toolkit/devtools/server/actors/storage.js
@@ +28,2 @@
>  // client (ms).
> +const BATCH_DELAY = 200;

What's the reasoning behind changing from 500 to 200ms?
Should we be worried about potential performance implications?
Does it mean that packets are sent every 200ms when the storage inspector is open? Do we make sure these packets aren't sent when the panel is inactive? Or are sending packets only there are changes?

@@ +59,5 @@
>  /**
>   * An async method equivalent to setTimeout but using Promises
>   *
>   * @param {number} time
> + *        The wait Time in milliseconds.

nit: s/Time/time

@@ +666,5 @@
>          args: args
>        });
>  
>        if (reply.length === 0) {
>          console.error("ERR_DIRECTOR_CHILD_NO_REPLY from " + methodName);

You've removed 2 throw statements here, is it ok if execution of the function continues in these 2 cases?

@@ +795,5 @@
> +        args: args
> +      });
> +    } catch(e) {
> +      // We may receive a NS_ERROR_NOT_INITIALIZED if the target window has
> +      // been closed. This can legitimately happen in between test runs.

It sounds like even if it can happen, it should be possible for us to fix the root cause when it does.
If that's correct, then please add a console.error in the catch so that we log something, and we take care of fixing the root cause (later, follow-up bug).

::: toolkit/devtools/server/tests/browser/browser_storage_updates.js
@@ +21,5 @@
> +      info('win.addCookie("c2", "foobar2")');
> +      win.addCookie("c2", "foobar2");
> +
> +      info('win.localStorage.setItem("l1", "foobar1")');
> +      win.localStorage.setItem("l1", "foobar1");

There's a lot of direct access to the content window in this TESTS array, which means, a lot of CPOWs usage, which we're trying to get rid of (see http://mzl.la/1Nn6e6m ).
I'm ok to do this in a separate bug because this was here before and we don't have a frame script setup in the test directory yet. But we should do it.

@@ +224,5 @@
> +    info("Still some updates pending for index " + index);
> +  }
> +}
> +
> +function* runTest({action, expected}, front, win, index) {

If runTest is called with yield in a Task, you either don't need the * if you make it return a promise, or you keep the * and make it use yield:

function* runTest({action, expected}, front, win, index) {
  let onUpdated = front.once("stores-update");
  action(win);
  let addedChangedDeleted = yield onUpdated;
  onStoresUpdate(expected, addedChangedDeleted, index);
}

Comment 16

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

Attachment: Patch 2: 1175850-storage-inspector-enable-tests.patch

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #15)
> Comment on attachment 8648019 [details] [diff] [review]
> Patch 2: 1175850-storage-inspector-enable-tests.patch
> 
> Review of attachment 8648019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Mike, this is a lot of work, and not easy work. Thanks for spending
> time on it.
> I didn't see anything major here, but I have some remarks and questions
> below, that I'd like answered before I R+
> 
> ::: browser/devtools/storage/test/browser_storage_sidebar.js
> @@ +100,5 @@
> > +add_task(function*() {
> > +  yield openTabAndSetupStorage(MAIN_DOMAIN + "storage-listings.html");
> > +
> > +  for (let test of testCases) {
> > +    let { location, sidebarHidden, sendEscape} = test;
> 
> nit: either no spaces after { or a space before } too.
> 

Done

> ::: browser/devtools/storage/test/head.js
> @@ +80,5 @@
> >   * @param url {String} The url to be opened in the new tab
> >   *
> >   * @return {Promise} A promise that resolves after storage inspector is ready
> >   */
> > +function* openTabAndSetupStorage(url) {
> 
> You made this a generator function while before it was a normal function
> that returned a promise.
> I think it's worth saying in the comment that it's a generator, it's easy to
> miss the * sign and call it with yield.
> 

Done

> @@ +198,5 @@
> >    Cu.forceCC();
> >    Cu.forceShrinkingGC();
> >  }
> >  
> > +function getAllWindows(baseWindow=gWindow) {
> 
> nit: add some jsdoc here please.
> 

Done

> @@ +484,5 @@
> >    gUI.tree.expandAll();
> > +
> > +  let animations = gUI.tree._parent.getAnimations();
> > +  for (let animation of animations) {
> > +    animation.finish();
> 
> A couple of questions about this:
> - are you sure this is fully implemented in the WebAnimations API? For some
> reason I'm getting errors when trying to call finish() on some simple
> animation test page. But I haven't ran this test yet.

It didn't show any errors but on testing I see that it doesn't actually do
anything. Works fine without intermittent oranges without this code so...

Removed.

> @@ +504,5 @@
> >   *        The id of the row in the table widget
> >   */
> > +function* selectTableItem(id) {
> > +  let target = gPanelWindow.document
> > +                 .querySelector(".table-widget-cell[data-id='" + id + "']");
> 
> nit: please align .querySelector with gPanelWindow on the line before.
> Or maybe make a variable for the selector so this line is shorter.
> 

Done

> ::: browser/devtools/storage/test/storage-complex-values.html
> @@ +82,5 @@
> >    console.log("added cookies and stuff from main page");
> >    callback();
> > +};
> > +
> > +function* deleteDB(dbName) {
> 
> If the function returns a promise, and is called with 'yield deleteDB(...)'
> within a Task, then there's no need to make that function a generator with
> the * character. It doesn't itself use yield.
> 

Of course, changed them back to normal functions.

> ::: browser/devtools/storage/test/storage-listings.html
> @@ +77,4 @@
> >    callback();
> > +};
> > +
> > +function* deleteDB(dbName) {
> 
> Same remark as in the previous file.
> 

Of course, changed them back to normal functions.

> ::: browser/devtools/storage/test/storage-secured-iframe.html
> @@ +54,4 @@
> >    callback();
> > +};
> > +
> > +function* deleteDB(dbName) {
> 
> And here.
> 

Of course, changed them back to normal functions.

> ::: browser/devtools/storage/ui.js
> @@ +84,5 @@
> > +    // The debugger server adds .from to packets for tracking purposes. Because
> > +    // we need to iterate through this object we simply delete .from as the
> > +    // performance hit when iterating over the object four times is
> > +    // inconsequential.
> > +    delete storageTypes.from;
> 
> After reading the comment above this line and after quickly going through
> the code, it's still not clear to me why you're deleting the .from property,
> so I suspect future readers of this code will ask themselves the same
> question.
> Wouldn't it be clearer if you just skipped the .from inside the for loop
> inside populateStorageTree?
> 

Moved to populateStorageTree.

> ::: toolkit/devtools/server/actors/storage.js
> @@ +28,2 @@
> >  // client (ms).
> > +const BATCH_DELAY = 200;
> 
> What's the reasoning behind changing from 500 to 200ms?

That is historic... the UX team used to say that we should wait a maximum of
200ms before showing changes as that looks instant to a user. Of course, there
could be further batches added but we still use 200ms as a base.

A lot of UX people say 100ms these days but 200ms seems just fine.

> Should we be worried about potential performance implications?

No, performance itself is improved. The old method used polling so it always
checked for changes every 500ms and submitted them. The new method waits 200ms
before submitting changes and resets this timer for each batch. In practice
200ms is plenty of time for a large amount of data.

1. Do we have data to send?
2. Start a 200ms timer.
3. Do we have more data to send? If so, reset the timer and goto 2.
4. If the timer finishes send the data.

> Does it mean that packets are sent every 200ms when the storage inspector is
> open?
Only if there is data to send each 200ms. Most of the time no data is sent... we
are no longer polling.

> Do we make sure these packets aren't sent when the panel is inactive?
> Or are sending packets only there are changes?
>
We are only sending packets when there are changes.

> @@ +59,5 @@
> >  /**
> >   * An async method equivalent to setTimeout but using Promises
> >   *
> >   * @param {number} time
> > + *        The wait Time in milliseconds.
> 
> nit: s/Time/time
> 

Corrected

> @@ +666,5 @@
> >          args: args
> >        });
> >  
> >        if (reply.length === 0) {
> >          console.error("ERR_DIRECTOR_CHILD_NO_REPLY from " + methodName);
> 
> You've removed 2 throw statements here, is it ok if execution of the
> function continues in these 2 cases?
> 

Yes, it means that the message manager has been removed by tool destruction.
Throwing at that point would prevent us from fully cleaning up after ourselves.

> @@ +795,5 @@
> > +        args: args
> > +      });
> > +    } catch(e) {
> > +      // We may receive a NS_ERROR_NOT_INITIALIZED if the target window has
> > +      // been closed. This can legitimately happen in between test runs.
> 
> It sounds like even if it can happen, it should be possible for us to fix
> the root cause when it does.
> If that's correct, then please add a console.error in the catch so that we
> log something, and we take care of fixing the root cause (later, follow-up
> bug).
> 

We are sending an async message here. Anything that causes the message manager
to be taken down between the message getting sent and receiving a result would
result in an NS_ERROR_NOT_INITIALIZED error.

Shutting down the browser or our tools is a typical cause of message manager
communication being terminated and will be the case for every async message
manager call we make.

I have logged bug 1195684.

> ::: toolkit/devtools/server/tests/browser/browser_storage_updates.js
> @@ +21,5 @@
> > +      info('win.addCookie("c2", "foobar2")');
> > +      win.addCookie("c2", "foobar2");
> > +
> > +      info('win.localStorage.setItem("l1", "foobar1")');
> > +      win.localStorage.setItem("l1", "foobar1");
> 
> There's a lot of direct access to the content window in this TESTS array,
> which means, a lot of CPOWs usage, which we're trying to get rid of (see
> http://mzl.la/1Nn6e6m ).
> I'm ok to do this in a separate bug because this was here before and we
> don't have a frame script setup in the test directory yet. But we should do
> it.
> 

Yes, I was planning on addressing this.

Logged bug 1195686.

> @@ +224,5 @@
> > +    info("Still some updates pending for index " + index);
> > +  }
> > +}
> > +
> > +function* runTest({action, expected}, front, win, index) {
> 
> If runTest is called with yield in a Task, you either don't need the * if
> you make it return a promise, or you keep the * and make it use yield:
> 
> function* runTest({action, expected}, front, win, index) {
>   let onUpdated = front.once("stores-update");
>   action(win);
>   let addedChangedDeleted = yield onUpdated;
>   onStoresUpdate(expected, addedChangedDeleted, index);
> }

Fixed

Comment 17

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

Attachment: Patch 1: 1175850-storage-inspector-eslint-fixes.patch

Comment 18

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

Ignore my comments about NS_ERROR_NOT_INITIALIZED... they are correct in the bug.

Comment 19

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

Attachment: Patch 2: 1175850-storage-inspector-enable-tests.patch

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc85aef65068

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8649203 [details] [diff] [review]
Patch 2: 1175850-storage-inspector-enable-tests.patch

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

Thanks Mike, I see all my comments addressed, and I don't think I have any other remarks.
Make sure you re-trigger dt test runs on that try build, to make sure there are no frequent intermittents.

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=211fdca474f3

Comment 23

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fb59be9f4a

Comment 24

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bffa058c8cb

Comment 25

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18d2d015cd36

Comment 26

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8da0b662c29

Comment 27

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

url:        https://hg.mozilla.org/integration/fx-team/rev/dfe22c74d4974e1e74de24d8cb2f51de44356b90
changeset:  dfe22c74d4974e1e74de24d8cb2f51de44356b90
user:       Michael Ratcliffe <[e-mail]>
date:       Tue Aug 18 11:48:22 2015 +0100
description:
Bug 1175850 - ESLint fixes r=me

url:        https://hg.mozilla.org/integration/fx-team/rev/b66a8e0c8e258a2c8d46774de0ddb2ecd62911c7
changeset:  b66a8e0c8e258a2c8d46774de0ddb2ecd62911c7
user:       Michael Ratcliffe <[e-mail]>
date:       Thu Aug 20 11:52:04 2015 +0100
description:
Bug 1175850 - Enable storage inspector tests after upgrading for E10S r=pbrosset

Comment 28

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

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfe22c74d497
https://hg.mozilla.org/mozilla-central/rev/b66a8e0c8e25