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.

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.

(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

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?

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. :(

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.

(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.

(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).

Something like this?

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.

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

(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.)

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 on attachment 8392107 [details] [diff] [review] Compatibility change only Review of attachment 8392107 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

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