753401 - The debugger server root and tab actors should be easily extensible

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

The discussion we had in Toronto concluded the following:

- The root actor should continue to offer a "listTabs" request. This is necessary because many other tools --- the network panel, the inspector, the style editor, and so on --- want to be able to be applied to a particular tab, so providing a place to hang tab-specific actor is a valuable service of the root actor.
- listTabs request: { "to":"root", "type":"listTabs" }
  reply: { "from":"root", "tabs": [tab, ...], "selected":index,
           "chromeDebugger":chromeDebuggerActor }
  where each tab is:
     { "title":title, "url":url,
       "debugger":contentDebuggerActor, "netPanel":netPanelActor,
       "inspector":inspectorActor, ... }
- I think we decided not to go with the listServers request, is that correct Jim?

What we have now is extensibility for the tab actor:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-browser-actors.js#437

This does not let the tab actor know the names and constructors of the added actors without a new API. Right now HttpMonitor and the Profiler rely on explicit knowledge of the actor names, instead of discovery.

Some actors, like the profiler or Honza's traceActor, need to cover the whole browser, not be constrained to a single tab. For those I think we need another array of actors in the listTabs response, like "globals" that would contain the chromeDebugger as well. The discoverability point applies here, too.

Does this seem like a sane extensibilty plan? Any missing pieces?

Comment 1

[Benoit Girard (:BenWa)]

Yes, isn't this bug 752896?

Comment 2

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #0)
> Does this seem like a sane extensibilty plan? Any missing pieces?

Yes, that sounds exactly right.

Just as you say, the 'tab' entries in the reply's 'tabs' property should actually name the additional actors available, so we don't have to 'just know' actor names.

I don't remember what 'listServers' was proposed as. But it does seem odd that a client would need to retrieve a tab list to find system-wide things like chrome debuggers and traceActor (link?). Was listServers meant to provide actor names like that? That would make sense to me.

Comment 3

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

(In reply to Benoit Girard (:BenWa) from comment #1)
> Yes, isn't this bug 752896?

As I tried to describe above that's not sufficient, unfortunately.

(In reply to Jim Blandy :jimb from comment #2)
> (In reply to Panos Astithas [:past] from comment #0)
> > Does this seem like a sane extensibilty plan? Any missing pieces?
> 
> Yes, that sounds exactly right.
> 
> Just as you say, the 'tab' entries in the reply's 'tabs' property should
> actually name the additional actors available, so we don't have to 'just
> know' actor names.
> 
> I don't remember what 'listServers' was proposed as. But it does seem odd
> that a client would need to retrieve a tab list to find system-wide things
> like chrome debuggers and traceActor (link?). Was listServers meant to
> provide actor names like that? That would make sense to me.

listServers appear in the whiteboard photo you took right on the top. IIRC it was part of our initial idea of how to do chrome debugging, but ISTR that it evolved to the stuff that's in the middle of the whiteboard. Hopefully your memory is less fuzzy than mine.

HttpMonitor has traceActor:

https://github.com/firebug/httpmonitor/blob/master/content/httpmonitor/server/traceActor.js

and netMonitorActor:

https://github.com/firebug/httpmonitor/blob/master/content/httpmonitor/server/netMonitorActor.js

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Note that bug 752896 has a patch for this already attached.
Honza

Comment 5

[Benoit Girard (:BenWa)]

If we land this bug I'm considering making the changes of bug 751034 in an extension. At least initial to allow faster prototype and update turn around. It would be lovely if you published a sample extension that adds a root actor.

Comment 6

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

Attachment: Patch v1

This is the direction I'm thinking of. Basically extensions register a name and a constructor function, either at global scope or tab scope, instead of a name and a handler, so that their presence can be deduced from protocol messages (in a REST-like way). This also should relieve the extension from handling lifetime issues of its actors, but I'll need to convert an extension to this model to be sure.

One small change from the initial plan is that, instead of returning the tab-scoped actors in the tab grip inside the "listTabs" response, I opted for putting them in the response of the "attach" request. This way these actors are initialized and placed into actor pools after the client picks a tab, instead of pre-allocating one actor for each tab, most of which would probably remain unused. Furthermore, the tab-scoped actor initialization happens inside the tab actor, where it belongs, instead of the parent root actor, like the global-scoped actors.

Another small refactoring that I think makes sense, is the removal of the "traits" array that was never used and the use of the "applicationType" field in the "hello" packet to differentiate between our various products/root actors. This can be used by the client to show or hide various UI elements, like the "chrome debugger" button in Fennec, or the list of tabs/threads in B2G.

I'm planning to start with dcamp's sample extension first and move to the profiler and HttpMonitor afterwards, if I get the time.

Comment 7

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #6)
> Another small refactoring that I think makes sense, is the removal of the
> "traits" array that was never used and the use of the "applicationType"
> field in the "hello" packet to differentiate between our various
> products/root actors. This can be used by the client to show or hide various
> UI elements, like the "chrome debugger" button in Fennec, or the list of
> tabs/threads in B2G.

I've changed the protocol docs to say that 'traits' can be omitted if empty.

Comment 8

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #6)
> Another small refactoring that I think makes sense, is the removal of the
> "traits" array that was never used and the use of the "applicationType"
> field in the "hello" packet to differentiate between our various
> products/root actors. This can be used by the client to show or hide various
> UI elements, like the "chrome debugger" button in Fennec, or the list of
> tabs/threads in B2G.

B2G vs. browser is definitely "application type" material, but I'd expect Fennec to describe itself as a browser, and use traits to specify its peculiarities.

You're saying that Fennec lacks chrome debugging (for now), right? If it's not convenient to wait for the listTabs response to decide what to do with the "chrome debugger" button, then the "traits" object (it's supposed to be an object, not an array) could say '"hasChromeDebugger":false'. That's the kind of thing it was intended for, anyway: characteristics of the server that can't be easily inferred from the responses to other packets.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 622782 [details] [diff] [review]
Patch v1

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

This looks great.

I wonder if we shouldn't put off actually calling constructors until we're sure we're going to use the actor. It seems a shame to construct (say) network tracing actors for a tab in those cases when we're going to ignore them and debug JS. Certainly, one could require all actors to be written frugally and initialize themselves lazily, but that's repeated work for every actor.

Perhaps, instead of calling new factory(arg, ...), we could initially store factory.bind(arg, ...) as the actor. Then, DebuggerServerConnection.prototype.onPacket could check whether typeof actor === "function", and if so, actually construct the actor, fix up its .conn and .actorID properties, and check if it needs to be added to its pool's _cleanups table.

It's a complication, but it sure would be nice to be able to mention actors the client might like to talk to without paying the cost of actually constructing them.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +79,5 @@
>     */
>    sayHello: function BRA_sayHello() {
> +    return {
> +      from: "root",
> +      applicationType: "browser-desktop" // "browser-fennec", "browser-b2g"

When we re-unify the root actors, it seems like this string should be something one supplies to the constructor.

Comment 10

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

(In reply to Jim Blandy :jimb from comment #8)
> (In reply to Panos Astithas [:past] from comment #6)
> > Another small refactoring that I think makes sense, is the removal of the
> > "traits" array that was never used and the use of the "applicationType"
> > field in the "hello" packet to differentiate between our various
> > products/root actors. This can be used by the client to show or hide various
> > UI elements, like the "chrome debugger" button in Fennec, or the list of
> > tabs/threads in B2G.
> 
> B2G vs. browser is definitely "application type" material, but I'd expect
> Fennec to describe itself as a browser, and use traits to specify its
> peculiarities.
> 
> You're saying that Fennec lacks chrome debugging (for now), right? If it's
> not convenient to wait for the listTabs response to decide what to do with
> the "chrome debugger" button, then the "traits" object (it's supposed to be
> an object, not an array) could say '"hasChromeDebugger":false'. That's the
> kind of thing it was intended for, anyway: characteristics of the server
> that can't be easily inferred from the responses to other packets.

I'm not sure that chrome debugging makes sense in Fennec, because AFAIK most of the app is native now and not in JS. But maybe having an option to debug this remaining JS part is important, so I can see why we could expose it.

My main reason for wanting to drop "traits" and go with applicationType is that I hadn't understood how the extension mechanism would work in practice. I assumed "traits" would be extra functions/actors the client would use for other tasks. If it's just a collection of flags, then it's certainly better than essentially encoding this information inside the "applicationType".

(In reply to Jim Blandy :jimb from comment #9)
> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +79,5 @@
> >     */
> >    sayHello: function BRA_sayHello() {
> > +    return {
> > +      from: "root",
> > +      applicationType: "browser-desktop" // "browser-fennec", "browser-b2g"
> 
> When we re-unify the root actors, it seems like this string should be
> something one supplies to the constructor.

Exactly. However, based on your previous points, I'm inclined to revert to the previous status quo.

Comment 11

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

(In reply to Jim Blandy :jimb from comment #9)
> I wonder if we shouldn't put off actually calling constructors until we're
> sure we're going to use the actor. It seems a shame to construct (say)
> network tracing actors for a tab in those cases when we're going to ignore
> them and debug JS. Certainly, one could require all actors to be written
> frugally and initialize themselves lazily, but that's repeated work for
> every actor.
> 
> Perhaps, instead of calling new factory(arg, ...), we could initially store
> factory.bind(arg, ...) as the actor. Then,
> DebuggerServerConnection.prototype.onPacket could check whether typeof actor
> === "function", and if so, actually construct the actor, fix up its .conn
> and .actorID properties, and check if it needs to be added to its pool's
> _cleanups table.
> 
> It's a complication, but it sure would be nice to be able to mention actors
> the client might like to talk to without paying the cost of actually
> constructing them.

It's a little more complex, but I don't mind that. My main question is this: since every client that needs to use a tab-scoped actor will need to attach to the tab first, is there any benefit from providing the extra actors before that? It seems to me that there is a sensible separation of concerns if the question of "which tab do I want?" is separated from "what do I want to debug in this tab (JS, network, etc.)?".

Comment 12

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

Attachment: Patch v2

Reverted changes to the "hello" packet and fixed actor registration. This seems to work for the sample-debug-actors and HttpMonitor:

https://github.com/past/sample-debug-actors
https://github.com/past/httpmonitor

I'm working on HttpMonitor now, and I need to make more changes to the client side to see if everything else works (actor lifetimes, etc.).

Comment 13

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #11)
> since every client that needs to use a tab-scoped actor will need to attach
> to the tab first, is there any benefit from providing the extra actors
> before that? It seems to me that there is a sensible separation of concerns
> if the question of "which tab do I want?" is separated from "what do I want
> to debug in this tab (JS, network, etc.)?".

I thought it might make for more pleasant user interaction. You don't want to make the user select a tab, and then tell her, "Oh, we can't attach to the profiler, you haven't installed the plugin" or whatever. You want to be able to say which services are available as soon as you connect.

Comment 14

[Jim Blandy :jimb]

Also, even once the user has selected the tab, I think we still don't want to create an instance of every possible service that tab offers. I guess I'm imagining that the kinds of actors mentioned in a 'tabAttached' response would usually be kind of heavy: entrain lots of stuff, and so on. If they're just a JS object with a few properties referring to an existing prototype, then that's no heavier than doing the 'bind' thing.

Comment 15

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

Attachment: Patch v3

This version uses the lazy loading approach suggested above. The modified sample debug actors (https://github.com/past/sample-debug-actors) work fine with this. I'm now validating the API with NetMonitor, and I still need to write tests and make sure that it doesn't break fennec and b2g, before asking for a review.

Comment 16

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

Attachment: Patch v4

Changed openListener/closeListener to support multiple concurrent clients connecting to the same debugger server. I'm using this successfully from (patched) NetMonitor, and it will be necessary for the web console and profiler as well.

Comment 17

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

Testing confirmed that this patch doesn't affect Fennec or B2G.

Comment 18

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

Attachment: Patch v5

Added tests and an API to unregister actors as well. I've modified NetMonitor and the Gecko Profiler to work with this API and it proved to be way cleaner than before. Jim I believe this satisfies all your previous comments, but do take a look and see if I missed anything.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #18)
> Created attachment 650908 [details] [diff] [review]
> Patch v5
> 
> I've modified NetMonitor
Could I see the changes? Or could you send a pull request to the repo here?
https://github.com/firebug/httpmonitor/commits/master

Thanks!
Honza

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 650908 [details] [diff] [review]
Patch v5

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

Alright, not a lot of comments for this. Just a few things that could be clarified and a couple of member variables that need to move into their constructors. I'm good with those changes and a try run.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +40,5 @@
>  
>  BrowserRootActor.prototype = {
> +
> +  // A map of actor names to actor constructors provided by extensions.
> +  _actorFactories: {},

should move this into the constructor. This is a dangerous pattern that's bit us before. Leaky!

@@ +235,5 @@
>    this._onWindowCreated = this.onWindowCreated.bind(this);
>  }
>  
>  // XXX (bug 710213): BrowserTabActor attach/detach/exit/disconnect is a
>  // *complete* mess, needs to be rethought asap.

still true.

@@ +274,5 @@
>    },
>  
> +  // A map of actor names to actor constructors provided by extensions.
> +  _actorFactories: {},
> +

nope. Still not liking it! :)

@@ +279,1 @@
>    actorPrefix: "tab",

these are constants, right? Maybe comment them to show that and explain why they're needed in dbg-server.js. (I had to go looking for this to see after reading about them in dbg-server.js).

@@ +289,5 @@
> +      title: this.browser.contentTitle,
> +      url: this.browser.currentURI.spec
> +    };
> +
> +    // Walk over tab actors added by extensions.

might like some additional explanation around "and add them to a new ActorPool".

@@ +301,5 @@
> +        actor.parentID = this.actorID;
> +        this._extraActors[name] = actor;
> +      }
> +      actorPool.addActor(actor);
> +      empty = false;

do we need a separate variable for this? Can we not just check the ActorPool if it hasActors? Might be a nice API call to have there. Pretty sure we can add it without breaking downstream consumers too.

@@ +311,5 @@
> +
> +    for (let name in this._extraActors) {
> +      let actor = this._extraActors[name];
> +      response[name] = actor.actorID;
> +    }

Seeing this for (let name in actors) pattern a lot in this patch. Feels like we could do something nicer than that, but my brain's not really seeing it this morning.

@@ +430,5 @@
>      this.conn.removeActorPool(this._tabPool);
>      this._tabPool = null;
> +    if (this._tabActorPool) {
> +      this.conn.removeActorPool(this._tabActorPool);
> +      this._tabActorPool = null;

should you call _tabActorPool.cleanup() here?

@@ +682,5 @@
> +    if (handler.name == aFunction.name) {
> +      delete BrowserRootActor.prototype._actorFactories[name];
> +    }
> +  }
> +};

Global Actor implies "everywhere". Is that the case? Adding a new global actor will augment the functionality of any inheritor of the RootActor? If so, we should be really explicit about that.

Looks good.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +447,5 @@
>                              error: "noSuchActor" });
>        return;
>      }
>  
> +    // Dyamically-loaded actors have to be created lazily, here.

I think the ", here" can go. :)

@@ +472,5 @@
>      if (actor.requestTypes && actor.requestTypes[aPacket.type]) {
>        try {
>          ret = actor.requestTypes[aPacket.type].bind(actor)(aPacket);
>        } catch(e) {
>          Cu.reportError(e);

ok!

Comment 21

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

(In reply to Jan Honza Odvarko from comment #19)
> (In reply to Panos Astithas [:past] from comment #18)
> > I've modified NetMonitor
> Could I see the changes? Or could you send a pull request to the repo here?
> https://github.com/firebug/httpmonitor/commits/master

For the record, I sent a pull request last week.

Comment 22

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

Attachment: Patch v6

(In reply to Rob Campbell [:rc] (:robcee) from comment #20)
> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +40,5 @@
> >  
> >  BrowserRootActor.prototype = {
> > +
> > +  // A map of actor names to actor constructors provided by extensions.
> > +  _actorFactories: {},
> 
> should move this into the constructor. This is a dangerous pattern that's
> bit us before. Leaky!

The reason I put the factories (and not the actor map) in the prototype, was to make sure an actor installed by a restartless add-on would instantly appear on extant actors. Putting the factories in the constructor would require new actors to be instantiated, which in the case of the root actor would mean a new connection.

However you make a valid point about the danger of getting memory leaks due to careless server extensions by add-on authors. It seems more important to guard against this and find a way to address the restartless problem with another API.

Initializing the factory store in the constructor didn't work, because the API methods are on DebuggerServer and have no references to the root and tab actors. The two options were to store the factories as a property of the constructor (BrowserRootActor._actorFactories and BrowserTabActor._actorFactories) or to store them as properties of the DebuggerServer (DebuggerServer._globalActorFactories and DebuggerServer._tabActorFactories). I picked the former in this patch, but we can use the latter if you prefer.

> @@ +301,5 @@
> > +        actor.parentID = this.actorID;
> > +        this._extraActors[name] = actor;
> > +      }
> > +      actorPool.addActor(actor);
> > +      empty = false;
> 
> do we need a separate variable for this? Can we not just check the ActorPool
> if it hasActors? Might be a nice API call to have there. Pretty sure we can
> add it without breaking downstream consumers too.

Good idea, added an ActorPool.isEmpty as discussed in IRC.

> @@ +430,5 @@
> >      this.conn.removeActorPool(this._tabPool);
> >      this._tabPool = null;
> > +    if (this._tabActorPool) {
> > +      this.conn.removeActorPool(this._tabActorPool);
> > +      this._tabActorPool = null;
> 
> should you call _tabActorPool.cleanup() here?

We are only ever calling pool.cleanup() when the connection is closed. Calling it in cases like this may help with the memory pressure, but I wouldn't want to mess with it in this bug. I'll file a followup.

> @@ +682,5 @@
> > +    if (handler.name == aFunction.name) {
> > +      delete BrowserRootActor.prototype._actorFactories[name];
> > +    }
> > +  }
> > +};
> 
> Global Actor implies "everywhere". Is that the case? Adding a new global
> actor will augment the functionality of any inheritor of the RootActor? If
> so, we should be really explicit about that.
> 
> Looks good.

I use the term Global to indicate that these actors are not tab-scoped. Only one such actor will be present in the server at any time. And no runtime actors inherit from the root actor, so new actors will only be available to clients for interaction, as separate individual entities.

Let me know if there is some comment that you'd like me to add, which would make this clearer.

I made all the other requested changes. I also think the patch satisfies all of Jim's feedback so far.

Comment 23

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

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=11635f91f5df

Comment 24

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

Attachment: Patch v7

(In reply to Panos Astithas [:past] from comment #22)
> Initializing the factory store in the constructor didn't work, because the
> API methods are on DebuggerServer and have no references to the root and tab
> actors. The two options were to store the factories as a property of the
> constructor (BrowserRootActor._actorFactories and
> BrowserTabActor._actorFactories) or to store them as properties of the
> DebuggerServer (DebuggerServer._globalActorFactories and
> DebuggerServer._tabActorFactories). I picked the former in this patch, but
> we can use the latter if you prefer.

So I changed my mind on this, and went back to DebuggerServer as we discussed yesterday, partly because of the leaks that appeared on try. I was hoping that this new version would plug them, but it didn't seem to:
https://tbpl.mozilla.org/?tree=Try&rev=8985e0cb232e

As I don't see any difference locally either with the patch applied or without, I'm still trying to figure out what is causing this.

Comment 25

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

Attachment: Patch v8

Fixed the leaks as can be seen in the last try: https://tbpl.mozilla.org/?tree=Try&rev=358bf3164608

For that I had to introduce a DebuggerServer.destroy() method that takes care of cleaning up the factory pools when the server is no longer used. Verified that HttpMonitor and the Profiler still work with these changes.

Comment 26

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

Full try run for all the related patches that I intend to land:
https://tbpl.mozilla.org/?tree=Try&rev=5061d5eb316d

Comment 27

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 658126 [details] [diff] [review]
Patch v8

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

alright, I think this has suffered enough.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +46,5 @@
>    sayHello: function BRA_sayHello() {
> +    return {
> +      from: "root",
> +      applicationType: "browser",
> +      traits: {}

are the traits included just for completeness or some other reason? Can be omitted, no?

@@ +290,5 @@
> +    let actorPool = new ActorPool(this.conn);
> +    for (let name in DebuggerServer.tabActorFactories) {
> +      let actor = this._extraActors[name];
> +      if (!actor) {
> +        actor = DebuggerServer.tabActorFactories[name].bind(null, this.conn);

this is making my head hurt.

If we get any FactoryFactories I'm leaving.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +86,5 @@
>      this.initTransport(aAllowConnectionCallback);
>      this.addActors("chrome://global/content/devtools/dbg-script-actors.js");
> +
> +    this.globalActorFactories = {};
> +    this.tabActorFactories = {};

good good. I like these here.

@@ +344,5 @@
>    /**
> +   * Returns true if the pool is empty.
> +   */
> +  isEmpty: function AP_isEmpty() {
> +    return Object.keys(this._actors).length == 0;

I really wish .length did the thing everybody wanted on objects. :/

Comment 28

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #27)
> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +46,5 @@
> >    sayHello: function BRA_sayHello() {
> > +    return {
> > +      from: "root",
> > +      applicationType: "browser",
> > +      traits: {}
> 
> are the traits included just for completeness or some other reason? Can be
> omitted, no?

It can be omitted if empty, yes. I left it there as a reminder for potential future Fennec changes (see comment 8), but also because it used to be an array in the code, whereas it was always an object in the spec.

Comment 29

[Jim Blandy :jimb]

Comment on attachment 658126 [details] [diff] [review]
Patch v8

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

I'm clearing the feedback? jimb flag on this patch. I haven't been able to put together the time to form a helpful opinion on it, but this is important stuff that a bunch of other work depends on. From what I have seen, I think it'll be fine to land this and iterate.

Comment 30

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

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

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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