931921 - Permissions table not displayed on device/simulator

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With the 7.0pre5dev version of the 1.3 Simulator add-on, the permission table doesn't appear after connecting.

Comment 1

[Paul Rouget [:paul]]

Simulator only?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It does work with the 2013-10-27 build of 1.3 on my Keon, so hopefully the Simulator just needs to be re-built.

Comment 3

[Alexandre Poirot [:ochameau]]

I'm planning to rebuild the simulator when we get the fix for the crash on Mac.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As of today, it appears I can now get the permission table from 1.3, but no longer from 1.2.  It doesn't seem like the simulator add-ons changed.  Not sure of the cause yet.

Comment 5

[Alexandre Poirot [:ochameau]]

Actually it's intermittent on 1.3 and always permanent on 1.2.
It is also related to the bug where we get the "connID function is undefined", having n/a everywhere in device panel and also not having the wallpaper.
We have facing race condition in the various messages we send to the device actor.
The message end up being mixed up and protocol.js throws exception as it associate the response of another request.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Prevent from creating multiple intances of global actors

That prevents responses packets for one actor to be sent in the same order than its requests.
(Per bug 895982)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is this ready for review?

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 828803 [details] [diff] [review]
Prevent from creating multiple intances of global actors

We should factor out global actors out of `_tabActorPool`. A new set of tab actors are expected to be instanciated on new call to listTabs, whereas global actors should only be instanciated once. Instanciating a new one on each call to listTabs end up mixing packets as bug 895982 patch only works when we have only one actor instance per actor id.

Comment 9

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

Comment on attachment 828803 [details] [diff] [review]
Prevent from creating multiple intances of global actors

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

The fix looks sane, but since this is core code, I'd like to have a unit test please. Perhaps something along the lines of test_add_actors.js that makes sure a tab list change doesn't result in new global actor IDs.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Patch with tests

https://tbpl.mozilla.org/?tree=Try&rev=edfa874be4f4
Actually, actor IDs were already unique, thanks to _extraActors.
So this patch introduces _globalActorPool which may be redundant with _extraActors.
But I wasn't able to fully clarify the very precise issue, packet ordering
is messed up if we remove'n add global actors in a new ActorPool.
I'd say that even if we have stable actor IDs, we end up with multiple actor instances
as actors will be spawn just before being added to its current pool:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#1010
Then we remove the pool, so we later reinstanciate another Actor.

In the test, I could ensure we get only one actor instance per connection,
but it feels weak:
  listTabs();
  let firstActor = DebuggerServer._connections[0].getActor(actorId);
  listTabs();
  check_eq(DebuggerServer._connections[0].getActor(actorId), firstActor);

So I prefered to only check ids... which were already correct before this patch!

Comment 11

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

Comment on attachment 8335293 [details] [diff] [review]
Patch with tests

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

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> In the test, I could ensure we get only one actor instance per connection,
> but it feels weak:
>   listTabs();
>   let firstActor = DebuggerServer._connections[0].getActor(actorId);
>   listTabs();
>   check_eq(DebuggerServer._connections[0].getActor(actorId), firstActor);
> 
> So I prefered to only check ids... which were already correct before this
> patch!

Well, this is one of the few cases where it actually makes sense to go around the protocol to check for actor identity, so I'd rather have the test check for that instead. r=me with that change.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Test actor instances

https://tbpl.mozilla.org/?tree=Try&rev=fe8a60e04504

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Fixed browser_dbg_globalactor.js

https://tbpl.mozilla.org/?tree=Try&rev=1795c348b2ca

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8339279 [details] [diff] [review]
Fixed browser_dbg_globalactor.js

Panos, I had to modify /browser/devtools/debugger/test/browser_dbg_globalactor.js
But at the end I'm not sure if I fixed the unit test, or introduce a behavior change? Tab actors and global actors are now in two distinct pools. But we do expect to have one instance of each type (tab and global), right?

Comment 15

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

Comment on attachment 8339279 [details] [diff] [review]
Fixed browser_dbg_globalactor.js

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

Yes, that's right, tab actors and global actors of the same type should be discrete instances.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This was backed out and re-landed while investigating a test failure.
https://hg.mozilla.org/integration/fx-team/rev/473ee8d452db

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/473ee8d452db