Closed Bug 1175850 Opened 5 years ago Closed 4 years ago

Enable storage inspector tests after upgrading for E10S

Categories

(DevTools :: Storage Inspector, defect)

defect
Not set

Tracking

(e10s+, firefox41 affected, firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
e10s + ---
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: miker, Assigned: miker)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 6 obsolete files)

No description provided.
Blocks: dte10s
tracking-e10s: --- → +
E10S still disabled... seems to be something to do with the message manager.
Also re-enable the storage inspector for browser/devtools/framework/test/browser_toolbox_window_shortcuts.js
Also:
browser/devtools/framework/test/browser_toolbox_tool_ready.js
browser/devtools/framework/test/browser_toolbox_tool_remote_reopen.js

Due to docshell leaks.
I'll hold off asking for review until try comes back but locally everything seems just peachy.
Attachment #8624789 - Attachment is obsolete: true
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
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
Attachment #8630982 - Attachment is obsolete: true
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
Finally green... I will tidy the code up a little more ready for review.
Depends on: 1194190
Blocks: 1194190
No longer depends on: 1194190
Sorry for the delay... had a leak after my latest pull.

ESLint Fixes.
Attachment #8648018 - Flags: review+
Comment on attachment 8648019 [details] [diff] [review]
Patch 2: 1175850-storage-inspector-enable-tests.patch

On second thoughts I already expect a green try.
Attachment #8648019 - Flags: review?(pbrosset)
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
Depends on: 1195135
Blocks: 1195135
No longer depends on: 1195135
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);
}
Attachment #8648019 - Flags: review?(pbrosset)
(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
Attachment #8648019 - Attachment is obsolete: true
Attachment #8649199 - Flags: review?(pbrosset)
Ignore my comments about NS_ERROR_NOT_INITIALIZED... they are correct in the bug.
Attachment #8649199 - Attachment is obsolete: true
Attachment #8649199 - Flags: review?(pbrosset)
Attachment #8649203 - Flags: review?(pbrosset)
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.
Attachment #8649203 - Flags: review?(pbrosset) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/dfe22c74d4974e1e74de24d8cb2f51de44356b90
changeset:  dfe22c74d4974e1e74de24d8cb2f51de44356b90
user:       Michael Ratcliffe <mratcliffe@mozilla.com>
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 <mratcliffe@mozilla.com>
date:       Thu Aug 20 11:52:04 2015 +0100
description:
Bug 1175850 - Enable storage inspector tests after upgrading for E10S r=pbrosset
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.