Closed Bug 1021775 Opened 5 years ago Closed 5 years ago

Bulk app upload never replies

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
Firefox 33
Tracking Status
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: jryans, Assigned: jryans)

Details

Attachments

(1 file)

In bug 924574, we added support for sending packaged apps via bulk data.  However, the upload actor never replies to say it's done.

We should add a reply here.  On a related note, consider whether the client should log an actual error in case a new request is sent while the same actor is still active[1].

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#792
How comes everything works well even if we don't get notified when the app has been installed?
It's only the bulk upload operation that does not get a reply. After that App Manager still needs to send the 'install' command to the webapps actor. I think currently App Manager doesn't wait for a reply to the bulk upload command, and therefore it just assumes that the bulk upload always succeeds and it's ok to proceed to asking the device to install what was just uploaded.
Yeah, app-actor-front queues up the install command once the *client* is done transferring the app via bulk data (without waiting to hear the server's status), and it's not possible for this message to be processed by the server until bulk app upload completes because bulk data is "greedy" and takes over the entire transport until it completes.

So, the App Manager does not rely on a reply after upload, but I think it should be added to follow the same pattern that other messages use.
At the moment, bulk upload waits for a reply to the upload message (because the debugger client wants a reply to every message).  So, let's give it one for completeness.

Try: https://tbpl.mozilla.org/?tree=Try&rev=387ecbe30040
Attachment #8441050 - Flags: review?(poirot.alex)
(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> Created attachment 8441050 [details] [diff] [review]
> Reply to bulk app install message
> 
> At the moment, bulk upload waits for a reply to the upload message (because
> the debugger client wants a reply to every message).  So, let's give it one
> for completeness.

Sorry, to clarify... we don't actually pause to literally "wait", but I just mean that debugger client expects some reply from every request it sends.  Without this, it's left waiting forever.
Attachment #8441050 - Flags: review?(poirot.alex) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9ebee16aea5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Comment on attachment 8441050 [details] [diff] [review]
Reply to bulk app install message

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature added in bug 924574
User impact if declined: Slight Dev Tools protocol difference when installing apps.  If declined, building external tools to communicate with FxOS devices to install apps is slightly harder, as there are more protocol variations to handle.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Attachment #8441050 - Flags: approval-mozilla-aurora?
Attachment #8441050 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.