Permissions table not displayed on device/simulator

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: WebIDE
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jryans, Assigned: ochameau)

Tracking

Trunk
Firefox 28
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

5 years ago
Simulator only?
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
I'm planning to rebuild the simulator when we get the fix for the crash on Mac.
(Reporter)

Comment 4

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → poirot.alex
Summary: Permissions table not displayed on 1.3 simulator → Permissions table not displayed on 1.2 device/simulator
(Assignee)

Comment 5

5 years ago
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.
Summary: Permissions table not displayed on 1.2 device/simulator → Permissions table not displayed on device/simulator
(Assignee)

Comment 6

5 years ago
Created attachment 828803 [details] [diff] [review]
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)
(Reporter)

Comment 7

5 years ago
Is this ready for review?
Flags: needinfo?(poirot.alex)
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
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.
Attachment #828803 - Flags: review?(past)
(Assignee)

Updated

5 years ago
Flags: needinfo?(poirot.alex)
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.
Attachment #828803 - Flags: review?(past)
(Assignee)

Comment 10

5 years ago
Created attachment 8335293 [details] [diff] [review]
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!
Attachment #828803 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8335293 - Flags: review?(past)
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.
Attachment #8335293 - Flags: review?(past) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 8339279 [details] [diff] [review]
Fixed browser_dbg_globalactor.js

https://tbpl.mozilla.org/?tree=Try&rev=1795c348b2ca
Attachment #8338629 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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?
Attachment #8339279 - Flags: review?(past)
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.
Attachment #8339279 - Flags: review?(past) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/eb8d750eabb3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
This was backed out and re-landed while investigating a test failure.
https://hg.mozilla.org/integration/fx-team/rev/473ee8d452db
https://hg.mozilla.org/mozilla-central/rev/473ee8d452db
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28

Updated

4 years ago
Duplicate of this bug: 952161
You need to log in before you can comment on or make changes to this bug.