1049888 - [e10s] Make the storage actor work in e10s and Firefox OS

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P1)

Description

[Girish Sharma [:Optimizer]]

There are a lot of issues right now in e10s mode :

 - cookieMgr.getCookiesFromHost - will have to write a helper method in
     parent process which listens to a message sent via contentMessageManager
     to that parent script. Something like :
     this.parentActor.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIContentFrameMessageManager)
                     .sendSyncMessage("devtools:storage:getCookiesFromHost", host)
 - OS.* - will have to move indexedDBActor.getDBNamesForHost to parent process like above
 - http-on-response-set-cookie - can be fixed by changing name of the event
     or whitelisting this particular event to work in e10s, See bug 806753

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Awesome, definitely good to have this as a follow-up. IMO not blocking though.

Comment 2

[Girish Sharma [:Optimizer]]

Totally, it should block the e10s bug instead.

Comment 3

[Girish Sharma [:Optimizer]]

mass component move . filter on #MassComponentMoveStorageInspector

Comment 5

[Chris Peterson [:cpeterson]]

These DevTools bugs should probably block e10s from riding to Aurora.

Comment 6

[Chris Peterson [:cpeterson]]

devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.

Comment 8

[Eddy Bruel [:ejpbruel]]

Mike, I've been having trouble reaching you on irc lately. How are we doing with this bug?

Comment 9

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

(In reply to Eddy Bruel [:ejpbruel] from comment #8)
> Mike, I've been having trouble reaching you on irc lately. How are we doing
> with this bug?

My apologies, it seems that my Adium installation is a little funky and disconnects invisibly... must be a feature. I really need to rebuild my mac but don't want to lose all of my settings.

I am doing fine but took time this week to work on a few other bugs. I had a lot of trouble getting communication from the child process to the parent working but apart from that I think it is going well. I will get back to working on it in the next few days and if I need any help I will ping you.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So, if I'm understanding this bug's flags, this was targeted to FF 40? Since that train has already left what's the next guess for a target version?

Comment 14

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

I am not happy with this as there is way too much communication between processes. Also, search for FIXME and you will see my problem... when handling a parent request we have no access to this.storageActor, which we need to use for updates... I am struggling to find a way around this.

Of course, there are other issues but once we have cookies updating themselves it should be easy to get other storage actors working.

Comment 15

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

Comment on attachment 8610536 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

Alex, do you have any general tips that would help here? In particularly the FIXME shows that this.storageActor is not available in the parent process... am I completely misusing setup(Parent|Child)Process or am I missing something.

In general any tips would be appreciated.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8610536 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

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

Unfortunately, you are understanding setupInParent correctly and you need such pipes between processes...
About the FIXME, I don't see any major issue or did I missed something?

The only suggestion I can make is to try to make like Honza, if possible, I don't know if it will be easy with storage.
He manage to factorize the code so that we ended up calling the exact same helper methond in e10s/non-e10s code.
Here it would mean calling the exact same helper method calling Services.cookies and Services.obs.addObserver.
I'm not completely sure it will be a life changer here, but it may help orginazing the code.

I thought we could do better by moving the actors completely in the parent process,
that to prevent this mess, but there is no TabActor in the parent,
so that there would not be any parentActor and its window-ready/window-destroyed event...
If you have ideas on how to improve that e10s mess at the framework/debugger server level I can help.
I just haven't found any better idea than this setupInParent helper and all these piping messages.
May be we could spawn special "sub actors" that works alone, that we can instanciate in the same process for non-e10s or in the parent for e10s.
Here, this sub actor would only implement getCookies/addObserver with a minimal number of arguments...

::: toolkit/devtools/server/actors/storage.js
@@ +521,5 @@
> +
> +          let cookie = msg.data;
> +
> +          // FIXME: We have no access to this.storageActor so we need some way
> +          // to ask for an update.

Hum, having access to `storageActor` here is just a scope issue, isn't it?
if you use arrow function or use a let self = this; in setupChildProcess first line, you should get access to it.

@@ +1488,5 @@
> +        dump("\n\naSubject:" + JSON.stringify(cookie) + "\n\n");
> +        dump("\n\naTopic:" + aTopic + "\n\n");
> +        dump("\n\naData:" + aData + "\n\n");
> +
> +        helpers.updateCookie(JSON.stringify(cookie));

Note that, here, you are going to send all cookies, for all documents. you should filter by host name.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8610536 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

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

The child-parent communication setup looks correct.
For info, I just filed bug 1168760 to re-word a little bit the documentation in /toolkit/devtools/server/docs/lazy-actor-modules.md
I hope this can help for this bug.

My suggestion:
There's unfortunately a lot of stuff that needs to be done in the parent process.
As Alex pointed out, the code would be simpler to write if everything could run in the parent process. But we can't do that.

One way to organize things in a way we can easily reason about the code is:
- move everything that actually accesses cookies, localStorage, ... in a separate set of helpers (classes, functions, objects, whatever), the goal being to isolate them in a way they can be called from anywhere,
- make a facade for these helpers that sets up the child-parent communication if needed, and exposes the same functions, and hiding the fact that it either goes through the MM or calls the implementation directly.

This way, the storage actor would almost be a shallow actor that relies on this facade to actually get things done.

As you know, we have about 3 weeks to get this into 41 when E10S becomes a reality for us. It looks to me like there is a lot of work to be done here. Maybe once you have put in place the general structure it'd be nice to have someone help on getting the localStorage, indexedDB working. What do you think?

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8610536 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

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

::: toolkit/devtools/server/actors/storage.js
@@ +521,5 @@
> +
> +          let cookie = msg.data;
> +
> +          // FIXME: We have no access to this.storageActor so we need some way
> +          // to ask for an update.

I agree with Alex, this function runs in the child process, when a message is received from the counterpart running in the parent process, so you should have access to the storageActor here without any problem.

Comment 19

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

We now have live cookie updates.

Comment 21

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

Now we have the cookie table working fine including live cookie updates under E10S and non-E10S setups.

@ochameau: pbrosset has asked that I get your feedback on this because you are familiar with setupParentProcess etc. I only have 3 weeks to get this landed and I have a bunch of storage types to add.

I have only dealt with the cookies table so far so there are a bunch to go but we want to be sure that you are happy with my approach in the first place. before I work on the other storage types.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8612869 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

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

Looks good overall you got it right, I just have various comment to help lowering the complixity of e10s support.

::: browser/devtools/storage/ui.js
@@ +402,5 @@
> +      }
> +      catch (ex) {
> +        json = null;
> +      }
> +    }

We are used to do that in devtools...
Do a potentialy costly operation that may fail surrounded by a try cast, but that can be terrible regarding performances.
Why value has so many patterns? couldn't we know which pattern we expect instead of try/catching all of them like this?

::: toolkit/devtools/server/actors/storage.js
@@ +660,5 @@
> +    // representing the cookie. If we have a real cookie we need to cast it into
> +    // an nsICookie2
> +    if (subject.QueryInterface) {
> +      subject = subject.QueryInterface(Ci.nsICookie2);
> +    }

To prevent such weird check, I would implement an helper function, similar to `inParentObserver` function, defined in `setupParentProcessForCookies`.
And rename `observe` here, to something like `onCookieChanged`.

@@ +722,5 @@
> +      };
> +      this.removeCookieObservers = () => {
> +        Services.obs.removeObserver(this, "cookie-changed", false);
> +        Services.obs.removeObserver(this, "http-on-response-set-cookie", false);
> +      };

That looks so hacky to define methods like this...
I'm wondering if instead of doing that...
we were just calling cookieHelpers directly.
But. We would define cookieHelpers conditonally.

let cookieHelpers = {
  // existing cookieHelpers definition
};
if (DebuggerServer.isInChildProcess) {
  const { sendSyncMessage, addMessageListener } = DebuggerServer.parentMessageManager;
  cookieHelper = {
    getCookiesFromHost: callParentProcess...,
    addObservers: ...,
    removeObservers: ...
  };
  // you would need to register the message listener in addObservers.
}
  }
}

Note that you could get rid of maybeSetupChildProcess if you prefer. Or you could define this.cookieHelper instead of overriding the cookieHelper global.

@@ +746,5 @@
> +
> +          this.observe(cookie, topic, data);
> +        break;
> +      }
> +    });

It would be perfect if you could unregister the listener on destroy or something. My previous suggestion with removeObservers on cookieHelpers should help.

@@ +775,5 @@
> +    let store = [];
> +
> +    while (cookies.hasMoreElements()) {
> +      let cookie = cookies.getNext().QueryInterface(Ci.nsICookie)
> +                          .QueryInterface(Ci.nsICookie2);

You copy pasted this code, but note that you only need to query nsICookie2 as nICookie2 extends nsICookie.

@@ +780,5 @@
> +      store.push(cookie);
> +    }
> +
> +    return JSON.stringify(store);
> +  }

So, I think you can move the addObservers here, as well as the inParentObserver that just does the QueryInterface.

  },
  addObservers: function(listener) {
    this.listener = listener;
    Services.obs.addObserver(this, "cookie-changed", false);
    Services.obs.addObserver(this, "http-...", false);
  },
  removeObservers: function () {
    Services.obs.removeObserver...
    this.listener = null;
  },
  observe: function(sub) {
    sub.QueryInterface(nsICookie2)
    this.listener.onCookieChanged(sub);
  }

@@ +842,5 @@
> +      case "cookie-changed":
> +      case "http-on-response-set-cookie":
> +        let cookie = aSubject.QueryInterface(Ci.nsICookie2);
> +
> +        helpers.observe(JSON.stringify(cookie), aTopic, aData);

There is no need for `helpers` object here,
you can directly call `callChildProcess` from here.
Also I would call it callChildProcess, but use something more specific, matching the event name being used ("storage:storage-cookie-request-child"). Something like `notifyCookieChanged`, emitCookieChanged, callCookieChanged,...

@@ +1553,5 @@
>      Services.obs.addObserver(this, "content-document-global-created", false);
>      Services.obs.addObserver(this, "inner-window-destroyed", false);
>      this.onPageChange = this.onPageChange.bind(this);
> +
> +    let handler = tabActor.docShell.chromeEventHandler;

Note the existence of tabActor.chromeEventHandler which is better than docShel.chromeEventHandler (some b2g bugfix...)

Comment 23

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

Comment 24

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

Comment 25

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

Attachment: 1049888-storage-actor-e10s.patch [WIP]

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> Comment on attachment 8612869 [details] [diff] [review]
> 1049888-storage-actor-e10s.patch [WIP]
> 
> Review of attachment 8612869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall you got it right, I just have various comment to help
> lowering the complixity of e10s support.
> 
> ::: browser/devtools/storage/ui.js
> @@ +402,5 @@
> > +      }
> > +      catch (ex) {
> > +        json = null;
> > +      }
> > +    }
> 
> We are used to do that in devtools...
> Do a potentialy costly operation that may fail surrounded by a try cast, but
> that can be terrible regarding performances.
> Why value has so many patterns? couldn't we know which pattern we expect
> instead of try/catching all of them like this?
> 

This is the value of the cookie and can be any ASCII string so the try catches are needed. We cover the most common value formats so that we can display them in key value pairs where appropriate:
- Key value pairs
- JSON
- JSON with names without quotes
- URL encoded JSON (often used for l10n)... cookie values are ASCII characters excluding CTLs, whitespace, DQUOTE, comma, semicolon and backslash... everything else gets mangled in theory.

> ::: toolkit/devtools/server/actors/storage.js
> @@ +660,5 @@
> > +    // representing the cookie. If we have a real cookie we need to cast it into
> > +    // an nsICookie2
> > +    if (subject.QueryInterface) {
> > +      subject = subject.QueryInterface(Ci.nsICookie2);
> > +    }
> 
> To prevent such weird check, I would implement an helper function, similar
> to `inParentObserver` function, defined in `setupParentProcessForCookies`.
> And rename `observe` here, to something like `onCookieChanged`.
> 

Done

> @@ +722,5 @@
> > +      };
> > +      this.removeCookieObservers = () => {
> > +        Services.obs.removeObserver(this, "cookie-changed", false);
> > +        Services.obs.removeObserver(this, "http-on-response-set-cookie", false);
> > +      };
> 
> That looks so hacky to define methods like this...
> I'm wondering if instead of doing that...
> we were just calling cookieHelpers directly.
> But. We would define cookieHelpers conditonally.

It is easier to bind between two objects (this and the helper) because in E10S we are effectively adding a We want the code to be called from the main actor in the same way whether we are running in E10S or not e.g. this.doSomething(). Using the methods directly from cookieHelpers would require:
1. Copy cookieHelpers.doSomething to cookieHelpers.mpDoSomething.
2. Replace cookieHelpers.doSomething with the message manager call to the parent process.
3. Parent process then calls cookieHelpers.mpDoSomething when it receives the "doSomething" message.

Maybe there is a way that is less hacky but I haven't found it... also see my next answer.

> 
> let cookieHelpers = {
>   // existing cookieHelpers definition
> };
> if (DebuggerServer.isInChildProcess) {
>   const { sendSyncMessage, addMessageListener } =
> DebuggerServer.parentMessageManager;
>   cookieHelper = {
>     getCookiesFromHost: callParentProcess...,
>     addObservers: ...,
>     removeObservers: ...
>   };
>   // you would need to register the message listener in addObservers.
> }
>   }
> }
> 
> Note that you could get rid of maybeSetupChildProcess if you prefer. Or you
> could define this.cookieHelper instead of overriding the cookieHelper global.
> 

I have spent a day playing around with this and other methods turn out even more hacky due to having to bind and manipulate contexts... especially when using observers. I have logged bug 1173324 to follow-up on this.

> @@ +746,5 @@
> > +
> > +          this.observe(cookie, topic, data);
> > +        break;
> > +      }
> > +    });
> 
> It would be perfect if you could unregister the listener on destroy or
> something. My previous suggestion with removeObservers on cookieHelpers
> should help.
> 

We already do that.

> @@ +775,5 @@
> > +    let store = [];
> > +
> > +    while (cookies.hasMoreElements()) {
> > +      let cookie = cookies.getNext().QueryInterface(Ci.nsICookie)
> > +                          .QueryInterface(Ci.nsICookie2);
> 
> You copy pasted this code, but note that you only need to query nsICookie2
> as nICookie2 extends nsICookie.
> 

Yeah, the original code needs some work... done.

> @@ +780,5 @@
> > +      store.push(cookie);
> > +    }
> > +
> > +    return JSON.stringify(store);
> > +  }
> 
> So, I think you can move the addObservers here, as well as the
> inParentObserver that just does the QueryInterface.
> 
>   },
>   addObservers: function(listener) {
>     this.listener = listener;
>     Services.obs.addObserver(this, "cookie-changed", false);
>     Services.obs.addObserver(this, "http-...", false);
>   },
>   removeObservers: function () {
>     Services.obs.removeObserver...
>     this.listener = null;
>   },
>   observe: function(sub) {
>     sub.QueryInterface(nsICookie2)
>     this.listener.onCookieChanged(sub);
>   }
> 

I very much prefer that... done.

> @@ +842,5 @@
> > +      case "cookie-changed":
> > +      case "http-on-response-set-cookie":
> > +        let cookie = aSubject.QueryInterface(Ci.nsICookie2);
> > +
> > +        helpers.observe(JSON.stringify(cookie), aTopic, aData);
> 
> There is no need for `helpers` object here,
> you can directly call `callChildProcess` from here.
> Also I would call it callChildProcess, but use something more specific,
> matching the event name being used ("storage:storage-cookie-request-child").
> Something like `notifyCookieChanged`, emitCookieChanged,
> callCookieChanged,...
> 

Fixed

> @@ +1553,5 @@
> >      Services.obs.addObserver(this, "content-document-global-created", false);
> >      Services.obs.addObserver(this, "inner-window-destroyed", false);
> >      this.onPageChange = this.onPageChange.bind(this);
> > +
> > +    let handler = tabActor.docShell.chromeEventHandler;
> 
> Note the existence of tabActor.chromeEventHandler which is better than
> docShel.chromeEventHandler (some b2g bugfix...)

Changed

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8617964 [details] [diff] [review]
1049888-storage-actor-e10s.patch [WIP]

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

There is many console.log everywhere and a bunch of commented code.
New code tweaks suggested.
I'm mostly conserned about http-on-response-set-cookie vanishing.

What about tests/try? Do have test for the storage actor? If yes, have you tried running them on e10s?

::: browser/devtools/storage/ui.js
@@ +402,5 @@
> +      }
> +      catch (ex) {
> +        json = null;
> +      }
> +    }

So, that has nothing to do with e10s, right?

I still find this method really weird and possibly unnecessary costly. We could at least check if the string starts with `{` before parsing json.
This seems to convert stuff automagically without really mentioning it. This is unexpected! I would expect the tool to just display the raw cookie by default. Or if converted by default we would provide a visual hint to notify it's not the raw value and eventually have a way to see the raw value!

What about keeping that for another patch/bug if that's not related to e10s?

::: toolkit/devtools/server/actors/storage.js
@@ -648,5 @@
> -        }
> -      }
> -      return null;
> -    }
> -

What happened to that code? I can't see it in new code after the patch?

@@ +655,5 @@
>        return null;
>      }
>  
>      let hosts = this.getMatchingHosts(subject);
> +    let data2 = {};

Could you keep it named `data`?

@@ +710,5 @@
> +      this.getCookiesFromHost = cookieHelpers.getCookiesFromHost;
> +      this.addCookieObservers = cookieHelpers.addCookieObservers;
> +      this.removeCookieObservers = cookieHelpers.removeCookieObservers;
> +      return;
> +    }

Ok so, if you don't want to face issues with `this` being wrong, you could replace `this.listener` by `cookieHelper.listener` in cookieHelper definition.

Then, here, you could just do:
if (!DebuggerServer.isInChildProcess) {
  this.cookieHelpers = cookieHelpers;
  return;
}
// or even better, define it by default on the prototype:
  cookieHelpers: cookieHelpers,
  maybeSetupChildProcess: function () {
    if (!DebuggerServer.isInChildProcess) return;
    ...

Then I think the rest should work like this:
  this.cookieHelpers = {
    getCookiesFromHost: callParentProcess.bind(null, "getCookiesFromHost"),
    addCookiesObservers: ...,
    removeCookieObservers: ...
  };

@@ +734,5 @@
> +          let [cookie, topic, data] = msg.data.args;
> +          this.onCookieChanged(cookie, topic, data);
> +        break;
> +      }
> +    });

That would be great if you can remove this message listener on detach/disconnect/destroy of the actor.
You may plug that into addCookieObservers/removeCookieObservers

let messageListener = msg => {
  ...
  this.onCookieChanged(...);
  ...
};
addCookieObservers: function (listener) {
  addMessageListener("storage:storage-cookie-request-child", messageListener);
  callParentProcess("addCookieObservers");
},
removeCookieObservers: function () {
  removeMessageListener("storage:storage-cookie-request-child", messageListener);
  callParentProcess("removeCookieObservers");
}

@@ +766,5 @@
> +      let cookie = cookies.getNext().QueryInterface(Ci.nsICookie2);
> +      store.push(cookie);
> +    }
> +
> +    return JSON.stringify(store);

Please avoid JSON.stringify/JSON.parse in non-e10s codepath, by moving stringify to handleChildRequest and parse to callParentProcess.

@@ +805,5 @@
> +  }
> +  gTrackedMessageManager.set("cookies", mm);
> +
> +  cookieHelpers.onCookieChanged =
> +    callChildProcess.bind(null, "onCookieChanged");

Rather than polluting cookieHelpers with no obvious reason, it would be clearer to create a dedicated object in handleChildRequest:
cookieHelpers.addCookieObservers({
  onCookieChanged: callChildProcess.bind(null, "onCookieChanged")
});

Comment 27

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

Attachment: 1049888-storage-actor-e10s.patch

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> Comment on attachment 8617964 [details] [diff] [review]
> 1049888-storage-actor-e10s.patch [WIP]
> 
> Review of attachment 8617964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is many console.log everywhere and a bunch of commented code.
> New code tweaks suggested.
> I'm mostly conserned about http-on-response-set-cookie vanishing.
> 

We don't need it any longer as cookies added with the Set-Cookie http header are now picked up by the cookie changed notification.

> What about tests/try?
> Do have test for the storage actor? If yes, have you
> tried running them on e10s?
> 

No they will need rewriting as we are now all async.

> ::: browser/devtools/storage/ui.js
> @@ +402,5 @@
> > +      }
> > +      catch (ex) {
> > +        json = null;
> > +      }
> > +    }
> 
> So, that has nothing to do with e10s, right?
> 
> I still find this method really weird and possibly unnecessary costly. We
> could at least check if the string starts with `{` before parsing json.
> This seems to convert stuff automagically without really mentioning it. This
> is unexpected! I would expect the tool to just display the raw cookie by
> default. Or if converted by default we would provide a visual hint to notify
> it's not the raw value and eventually have a way to see the raw value!
> 
> What about keeping that for another patch/bug if that's not related to e10s?
> 

I always planned on that... bug 1173701 logged.

> ::: toolkit/devtools/server/actors/storage.js
> @@ -648,5 @@
> > -        }
> > -      }
> > -      return null;
> > -    }
> > -
> 
> What happened to that code? I can't see it in new code after the patch?
> 

I removed the "http-on-response-set-cookie" returned an nsIChannel but it turns out that there is no way to use it across processes as it is not possible to discover the originating window. It turned out that we never needed it because these cookies also raise a "cookie-changed" notification.

> @@ +655,5 @@
> >        return null;
> >      }
> >  
> >      let hosts = this.getMatchingHosts(subject);
> > +    let data2 = {};
> 
> Could you keep it named `data`?
> 

Done

> @@ +710,5 @@
> > +      this.getCookiesFromHost = cookieHelpers.getCookiesFromHost;
> > +      this.addCookieObservers = cookieHelpers.addCookieObservers;
> > +      this.removeCookieObservers = cookieHelpers.removeCookieObservers;
> > +      return;
> > +    }
> 
> Ok so, if you don't want to face issues with `this` being wrong, you could
> replace `this.listener` by `cookieHelper.listener` in cookieHelper
> definition.
> 
> Then, here, you could just do:
> if (!DebuggerServer.isInChildProcess) {
>   this.cookieHelpers = cookieHelpers;
>   return;
> }
> // or even better, define it by default on the prototype:
>   cookieHelpers: cookieHelpers,
>   maybeSetupChildProcess: function () {
>     if (!DebuggerServer.isInChildProcess) return;
>     ...
> 
> Then I think the rest should work like this:
>   this.cookieHelpers = {
>     getCookiesFromHost: callParentProcess.bind(null, "getCookiesFromHost"),
>     addCookiesObservers: ...,
>     removeCookieObservers: ...
>   };
> 

That doesn't work... getCookiesFromHost: callParentProcess.bind(null, "getCookiesFromHost") overwrites the method with a message call with the method name. This overwrites the original method so we would have to make a copy of the original method somewhere and that quickly gets very messy and way more hacky than what we currently have. We also want to be able to use this.methodName in both E10S and non-E10S code inside our actors.

I have logged bug 1173324 to follow-up on this but for now I think we should stick with what works so that our users have something rather than nothing.

> @@ +734,5 @@
> > +          let [cookie, topic, data] = msg.data.args;
> > +          this.onCookieChanged(cookie, topic, data);
> > +        break;
> > +      }
> > +    });
> 
> That would be great if you can remove this message listener on
> detach/disconnect/destroy of the actor.
> You may plug that into addCookieObservers/removeCookieObservers
> 
> let messageListener = msg => {
>   ...
>   this.onCookieChanged(...);
>   ...
> };
> addCookieObservers: function (listener) {
>   addMessageListener("storage:storage-cookie-request-child",
> messageListener);
>   callParentProcess("addCookieObservers");
> },
> removeCookieObservers: function () {
>   removeMessageListener("storage:storage-cookie-request-child",
> messageListener);
>   callParentProcess("removeCookieObservers");
> }
> 

addCookieObservers needs to run in the parent process whilst the adding of the messageListener needs to run in the child process so we can't do it there.

> @@ +766,5 @@
> > +      let cookie = cookies.getNext().QueryInterface(Ci.nsICookie2);
> > +      store.push(cookie);
> > +    }
> > +
> > +    return JSON.stringify(store);
> 
> Please avoid JSON.stringify/JSON.parse in non-e10s codepath, by moving
> stringify to handleChildRequest and parse to callParentProcess.
> 

Done.

> @@ +805,5 @@
> > +  }
> > +  gTrackedMessageManager.set("cookies", mm);
> > +
> > +  cookieHelpers.onCookieChanged =
> > +    callChildProcess.bind(null, "onCookieChanged");
> 
> Rather than polluting cookieHelpers with no obvious reason, it would be
> clearer to create a dedicated object in handleChildRequest:
> cookieHelpers.addCookieObservers({
>   onCookieChanged: callChildProcess.bind(null, "onCookieChanged")
> });

I don't see any advantage of doing this over our current method.

Comment 28

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

Attachment: 1049888-storage-actor-e10s.patch

A lot of changes were cosmetic but I have written a walkthrough to help you see how this works.

Cookies
=======
maybeSetupChildProcess() (Line 636)
  - transplant methods from cookieHelpers if we are in the parent process.
  - If we are in the child process set up the message manager to send messages to the parent process to call methods.
This means that this.method() would work differently depending on which process called it:
  - In the parent process:
    - Call this.method that was transplanted from cookieHelpers.method()
  - In the child process:
    - Call this.method that sends a message to the parent process via the message manager. The method is then called directly in cookieHelpers.method()

cookieHelpers (Line 694) is really a collection of methods designed to run in the parent process.

exports.setupParentProcessForCookies() (line 730) simply sets up the parent process side of things.

ObjectStoreMetadata (Line 941) has been changed to use an array rather than a map and create a serializable copy of itself (maps are destroyed by JSON.stringify)

indexedDB
=========
DatabaseMetadata (Line 986) has been changed to use an array rather than a map.

maybeSetupChildProcess() (Line 1194) Same as the method in the Cookie section but this works with indexedDB methods.

indexedDBHelpers (Line 1254) These methods have only been slightly changed in order to make them work in the parent process. The notable methods are:
  - backToChild() (Line 1255) Send a message back to the content process containing a method name and some params. This in combination with callParentProcessAsync() (Line 1239) allows us to:
    - yield in the child process using yield this.method()
    - wait for a method to complete *in the parent process* via this.backToChild("asyncMethodName", params);
    - Receive the relevant data in the content process and continue execution.
  - patchMetadataMapsAndProtos() Because the objects we build in the chrome process are serialized and sent to the content process they lose their prototypes. This method reinstates these prototypes and converts the arrays that used to be Maps back into maps. It is a little hacky but until we have a better way to pass complex objects between processes this is the best we can do.
  - getObjectStoreData() (Line 1460) needed a little help to return values that we could use across processes via the message manager.

exports.setupParentProcessForIndexedDB (Line 1550) is pretty much the same as exports.setupParentProcessForCookies() (line 730)

Other Stuff
===========
- I removed the "http-on-response-set-cookie" returned an nsIChannel but it turns out that there is no way to use it across processes as it is not possible to discover the originating window. It turned out that we never needed it because these cookies also raise a "cookie-changed" notification.

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

Comment 29

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

Attachment: 1049888-storage-actor-e10s.patch

Generally tidied up and a little housecleaning.

I feel comfortable landing this and only running the non-e10s tests... depending on what try looks like:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ca709d9d90

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8624105 [details] [diff] [review]
1049888-storage-actor-e10s.patch

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

I didn't do an exhaustive verification of the changes as I'm not familiar with this actor and Splinter didn't help me figure out the moved bits from the modified ones. I read the patch thoroughly however and I have some comments on the parts that I understand.

Apart from that, I saw quite a few errors in the browser console while testing the patch on cnn.com. Here is a sample:

Sending message that cannot be cloned. Are you trying to send an XPCOM object?  storage.js:780:0

NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface] storage.js:719:0

TypeError: deferred is undefined storage.js:1233:11

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/devtools/ViewHelpers.jsm :: ViewHelpers.L10N.prototype.getStr :: line 325"  data: no] Promise-backend.js:923:0

The worst case was when I opened a new tab, typed cnn.com<ENTER> and then quickly opened the storage panel, before the page being loaded.

Error occurred while creating actor 'undefined: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsISyncMessageSender.sendSyncMessage]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js :: DebuggerServer.setupInParent :: line 938"  data: no]
Stack: DebuggerServer.setupInParent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:938:1
.maybeSetupChildProcess@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js:648:1
.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js:477:5
constructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js:146:23
exports.StorageActor<.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js:1660:38
constructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js:146:23
ObservedActorFactory.prototype.createActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:115:18
DebuggerServerConnection.prototype._getOrCreateActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1440:16
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1584:17
ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:742:5
Line: 938, column: 0 main.js:1467:0

For some reason that broke the storage panel permanently until browser restart. Other panels (console, debugger) continued to function however. Fortunately I couldn't reproduce this in non-e10s mode, so it doesn't look like a regression.

I expect that having tests would have caught some of these issues and we would have fixed them. It's disconcerting to ship code with that many bugs. On the other hand, this patch takes an important feature from not working at all in e10s to a functional, albeit incomplete, state. Since the behavior of the storage panel seems better in non-e10s mode, I think we can land this and iterate quickly afterwards. It will be certainly easier to uplift more contained bug fixes later if need be, than to crash land the entire rewrite in aurora.

So with all that in mind I'm giving it an r+ despite the relatively large number of identified pending issues.

::: browser/devtools/storage/test/browser.ini
@@ +1,5 @@
>  [DEFAULT]
>  tags = devtools
>  # Bug 1049888 - storage actors do not work in e10s for now
>  # Bug 1105803 - permafailing on debug release builds since devedition landed
> +skip-if = true

I think you mentioned that you got the tests working in non-e10s mode, but there are still issues when run in e10s. In that case it would be better to land what you have now, even with skip-if = e10s, so that we have some form of reassurance that we don't regress the storage inspector.

If my recollection is wrong, please add the bug# where the tests will be reinstated.

::: toolkit/devtools/server/actors/storage.js
@@ +21,2 @@
>  
>  Cu.import("resource://gre/modules/Promise.jsm");

require("Promise") in server code please.

@@ +23,1 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");

XPCOMUtils can be imported lie this:

const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");

But you don't really need it, since we already have loader.lazyImporter for what you want it for:

loader.lazyImporter(this, "OS", "resource://gre/modules/osfile.jsm");

This way you can remove all instances of Cu.import from the actor.

@@ +76,1 @@
>    let wait = Promise.defer();

Ugh, this file doesn't use lower case |promise| like the rest of devtools. Not your fault, but it would be nice to see it fixed if it's not too much work.

@@ +553,5 @@
>    populateStoresForHost: function(host) {
>      this.hostVsStores.set(host, new Map());
> +
> +    // let cookieJSON = this.getCookiesFromHost(host);
> +    // let cookies = JSON.parse(cookieJSON);

These must be debugging leftovers, right? Don't forget to remove them.

@@ +673,5 @@
> +      });
> +
> +      if (reply.length === 0) {
> +        console.error("ERR_DIRECTOR_CHILD_NO_REPLY");
> +        throw Error("ERR_DIRECTOR_CHILD_NO_REPLY");

I got this error (and a stack trace) when closing the toolbox. We should handle that case gracefully, but I'm fine with it done in a followup if you prefer.

@@ +731,5 @@
> +  // prevents multiple subscriptions on the same messagemanager
> +  if (gTrackedMessageManager.has("cookies")) {
> +    return;
> +  }
> +  gTrackedMessageManager.set("cookies", mm);

Wouldn't it be better to set this once the setup is done, so in case of failure in the following code gTrackedMessageManager doesn't believe it's left with a working subscription?

@@ +801,5 @@
>        let storage = this.hostVsStores.get(host);
>        if (name) {
>          return [{name: name, value: storage.getItem(name)}];
>        }
> +      return Object.keys(storage).map(name2 => {

How about |key| instead of |name2|?

@@ +1360,5 @@
> +    return host.replace(ILLEGAL_CHAR_REGEX, "+");
> +  },
> +
> +  /**
> +   * Retrives the proper indexed db database name from the provided .sqlite file

Typo: Retrieves

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #29)
> Created attachment 8624788 [details] [diff] [review]
> 1049888-storage-actor-e10s.patch
> 
> Generally tidied up and a little housecleaning.
> 
> I feel comfortable landing this and only running the non-e10s tests...
> depending on what try looks like:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ca709d9d90

Ah, that's why I got an error while posting the review. I don't expect these changes to require additional review, right?

Agreed on the non-e10s tests being enabled for now, as I mentioned above.

Comment 32

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

Attachment: 1049888-storage-actor-e10s.patch

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

Based on:
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=324f8652a187

Hopefully I am based on a green tree this time.

Comment 33

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

Attachment: 1049888-storage-actor-e10s.patch

Fixed the failing test am am very confident of a green try but as long as fx-team is down we may as well wait for the try results.

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

Comment 34

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

Attachment: 1049888-storage-actor-e10s.patch

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

Comment 35

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

Attachment: 1049888-storage-actor-e10s.patch

Finally all green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7d444357b1

Comment 36

[Pulsebot]

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

Comment 37

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

Comment on attachment 8625085 [details] [diff] [review]
1049888-storage-actor-e10s.patch

Fixed in fx-team.
https://hg.mozilla.org/integration/fx-team/rev/ac02ae385e75

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac02ae385e75

Comment 40

[Ian Nartowicz]

Setting and removing items individually looks good and is reflected in the console, but it doesn't seem to respond to a storage area clear() call.