1061653 - Remote profiler doesn't work with old geckos

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

Description

[Alexandre Poirot [:ochameau]]

Most likely when bug 879008 landed, we broke the support of profiling previous gecko releases remotely (fxos, fennec tabs, ...).
I can see various failures. I tried to fix the simple ones but it looks like there is many incompatibilities between recent front/client versus old actors.

One example:
error occurred while processing 'startProfiler: TypeError: aRequest.features is undefined
Stack: ProfilerActor.prototype.onStartProfiler
resource://gre/modules/devtools/server/actors/profiler.js:10:133

Recent front doesn't send `features` attribute anymore, whereas old actor expect one.

Second example:
unrecognizedPacketType for getPendingTicks and cancelRecording when stopping the profile.

I stopped my investigation here.

Comment 1

[Paul Rouget [:paul]]

I see 2 options here:
- we make the new frontend compatible with the old backend
- if too hard, we decide to only enable the profiler for the new backend

Victor, can I ask you to look at this and what's the best strategy here?

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to Alexandre Poirot [:ochameau] from comment #0)
>
> One example:
> error occurred while processing 'startProfiler: TypeError: aRequest.features
> is undefined
> Stack: ProfilerActor.prototype.onStartProfiler
> resource://gre/modules/devtools/server/actors/profiler.js:10:133
> 
> Recent front doesn't send `features` attribute anymore, whereas old actor
> expect one.
> 

Fixing the profiler actor is trivial at most.

> Second example:
> unrecognizedPacketType for getPendingTicks and cancelRecording when stopping
> the profile.
> 
> I stopped my investigation here.

This is because framerate actors don't exist on old versions. We could make the frontend adapt in this case and not show such graphs. There is another problem to take into consideration: the fact that C++ profile entries don't have category information on old geckos, but that's about it.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8491749 [details] [diff] [review]
v1

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

Tested with Firefox for Android (beta 33, updated 2 days ago), recording starts fine, but when I click again to stop it:

A coding exception was thrown and uncaught in a Task.
Full message: TypeError: CATEGORY_MAPPINGS[category] is undefined
Full stack: RecordingUtils.plotCategoriesFor@chrome://browser/content/devtools/ui-profile.js:626:13
ProfileView.populateTab<@chrome://browser/content/devtools/ui-profile.js:160:26
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
ProfileView.addTabAndPopulate<@chrome://browser/content/devtools/ui-profile.js:192:11
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

Also tested with Firefox desktop 32:

Protocol error (unrecognizedPacketType): Actor conn0.framerate53 does not recognize the packet type getPendingTicks

So, although code changes look good, there seem to be incompatibilities still with versions earlier than 34.

::: browser/devtools/profiler/utils/global.js
@@ +45,5 @@
>  };
>  
> +// Human-readable "other" category bitmask. Older Geckos don't have all the
> +// necessary instrumentation in the sampling profiler backend for creating
> +// a categories graph, in which case the we default to the "other" category.

There's an extra "the" in this sentence.

Comment 5

[Victor Porof [:vporof][:vp]]

That's really weird, I tested this with fx 32, 33, and 34...
I'll test again.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v2

Alright, this should do it! Good eye catching those failures Patrick!

Comment 7

[Victor Porof [:vporof][:vp]]

Btw, with regards to "Protocol error (unrecognizedPacketType): Actor conn0.framerate77 does not recognize the packet type getPendingTicks", I'm deferring the fix to bug 1069673, since there's currently no good way of verifying this.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8492202 [details] [diff] [review]
v2

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

Tested successfully on firefox for android 33.
Still failing on firefox desktop 32 (Protocol error (unrecognizedPacketType): Actor conn0.framerate53 does not recognize the packet type getPendingTicks), but if you plan to fix the remaining issue in bug 1069673 that's fine.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a221e7030b1
https://hg.mozilla.org/mozilla-central/rev/d1ff54346db5

Comment 11

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

Victor, I believe this is also broken in 34, since that's when the new profiler landed.

Could we uplift this to Aurora?

Comment 12

[Victor Porof [:vporof][:vp]]

Definitely!

Comment 13

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

Comment on attachment 8492202 [details] [diff] [review]
v2

Approval Request Comment
[Feature/regressing bug #]: Regressed when new profiler from bug 879008 landed
[User impact if declined]: Profiler won't work with older b2g devices
[Describe test coverage new/current, TBPL]: Landed on m-c, working there
[Risks and why]: Low risk, just improves backward compatibility
[String/UUID change made/needed]: None

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8492202 [details] [diff] [review]
v2

Aurora+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a14a70a14620