982597 - Remove backwards compatibility for the "sources" packet

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[:Paolo Amadini]

The "protocol.js" in the debugger server needs a special treatment in the Promises conversion, because it relies on the interaction between the event loop and Add-on SDK Promises to send packets in the expected order.

Comment 1

[:Paolo Amadini]

Attachment: Tentative patch

So, the original problem with the order of protocol packets has since been resolved in the server, the issue here was unrelated.

The problem is that the ProtocolCompatibility object works asynchronously, but clients expect the function generated by DebuggerClient.requester in getSources to be executed synchronously. This was causing the debugger client to display wrong source files.

I'm not sure about how to solve this issue. This patch has a workaround that I'm just using to check if there are other issues, to solve this one maybe we could refactor the ProtocolCompatibility object to avoid using promises, or understand why the function generated by DebuggerClient.requester needs to be called synchronously.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to :Paolo Amadini from comment #1)
> The problem is that the ProtocolCompatibility object works asynchronously,
> but clients expect the function generated by DebuggerClient.requester in
> getSources to be executed synchronously. This was causing the debugger
> client to display wrong source files.

Can you expand on this? RDP packets are always sent asynchronously, even in the case when you are debugging locally: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/transport.js#252

Comment 3

[:Paolo Amadini]

The problem is with the call to "this.compat.supportsFeature("sources").then" that I commented out in the attached patch, I don't know if that is actually related to the protocol.

Currently, the callback to "then" is executed immediately, before the getSources function returns. After switching to Promise.jsm, the callback is called after getSources returns, either in the next tick of the event loop or in the same tick during an outer promise resolution loop. This is apparently the cause of the problem.

I examined the obvious fact that the getSources property on the object now is updated later. If we don't change the getSources property, and just invoke the function generated by DebuggerClient.requester with the correct reference to "this", after the original getSources function returned, the behavior of the debugger client is incorrect and various browser-chrome tests fail.

If we do the same call synchronously without updating the property, the code still works.

Ideas?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Troubling.

Order should be the same whether async or sync because the |.then()| which calls the patched version of |getSources| is |.then()| chained after the patching.

It shouldn't affect |getSources| callers because the |aOnResponse| callback is async anyways because it is called after an RDP request (which enforces async).

(In reply to :Paolo Amadini from comment #3)
> I examined the obvious fact that the getSources property on the object now
> is updated later. If we don't change the getSources property, and just
> invoke the function generated by DebuggerClient.requester with the correct
> reference to "this", after the original getSources function returned, the
> behavior of the debugger client is incorrect and various browser-chrome
> tests fail.

This is very troubling. That should totally work, albeit performing unnecessary extra compatibility checks.

:(

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Come to think of it, I am 99% sure we can remove this case of compatibility checking. This is done to support Firefox <22, and the last ESR was 24 and B2G 1.3 is on 28.

Will give a quick r+ if you redefine |getSources| as the compatible |DebuggerClient.requester| version of the function. Hopefully that should solve our problems here, as well.

Comment 6

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> Will give a quick r+ if you redefine |getSources| as the compatible
> |DebuggerClient.requester| version of the function. Hopefully that should
> solve our problems here, as well.

Sounds good! I guess this makes the supportsFeature function unused, not sure if I should file a follow-up bug to take care of that in the future.

Comment 7

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> Come to think of it, I am 99% sure we can remove this case of compatibility
> checking. This is done to support Firefox <22, and the last ESR was 24 and
> B2G 1.3 is on 28.

...and debugging JS in Firefox OS 1.1 (Gecko 18) is disabled anyway.

We should also remove the "sources" trait and associated machinery in that case (not necessarily the ProtocolCompatibility code, although everyone else is just adding traits).

Comment 8

[:Paolo Amadini]

Attachment: The patch

Something like this?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8390480 [details] [diff] [review]
The patch

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

Does this still have the test failures you were seeing before?

::: toolkit/devtools/client/dbg-client.jsm
@@ +1529,5 @@
> +  getSources: DebuggerClient.requester({
> +    type: "sources"
> +  }, {
> +    telemetry: "SOURCES"
> +  }),

Yup!

::: toolkit/devtools/server/tests/unit/test_protocol_async.js
@@ +142,4 @@
>      }));
>  
>      calls.push(rootClient.simpleReturn().then(ret => {
> +      return deferAfterRejection.promise.then(function () {

Seems like you could simplify a bunch of these wait-on-two-promises things to |Promise.all([...])| calls.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Also, as Panos said, please remove the "sources" trait from the RootActor's hello packet.

Comment 11

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> >      calls.push(rootClient.simpleReturn().then(ret => {
> > +      return deferAfterRejection.promise.then(function () {
> 
> Seems like you could simplify a bunch of these wait-on-two-promises things
> to |Promise.all([...])| calls.

Given that this test still depends on the exact timing of "then" processing, I think it's better to have simple calls here. (In general, when using any implementation of promises, the only way to be sure about processing order is with a queue or dependency graph, but this would be out of scope for this conversion.)

Comment 12

[:Paolo Amadini]

Attachment: Compatibility change only

I've morphed this bug to be about the compatibility change only, as the test changes are better handled together with the ones in bug 943517. I've removed the specific compatibility code that I missed before.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8392107 [details] [diff] [review]
Compatibility change only

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

Thanks!

Comment 14

[:Paolo Amadini]

Tryserver green for the relevant tests:

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

I think the other tests should work fine, pushing directly to fx-team to save resources:

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

Comment 15

[Phil Ringnalda (:philor)]

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