Closed Bug 1226156 Opened 6 years ago Closed 6 years ago

Multiple requests and updates happening when opening a Loop room

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
5

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.2 - Nov 30
Tracking Status
firefox45 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: perf, regression)

Attachments

(1 file, 1 obsolete file)

STR:

1) Start up FF, set loop.debug.dispatcher = true in prefs, restart.
2) Open the web console.
3) Open the panel
4) Open an existing room
5) Watch the output

Actual results

=> Multiple items are logged along the lines of:

[Dispatcher] Dispatching action Object { roomList: Array[22], name: "updateRoomList" }

=> Looking through the history, there's also two GET requests to http://localhost:5000/v0/rooms for when the room was openend.

Expected Results

=> only one line for dispatching the "updateRoomList" action
=> only one GET request

This was introduced at some stage when we started updating the tooltip of the Hello button - two calls to LoopRooms#getAll are happening at the same time. The rough details of what's happening currently are:

- The first call is caused by the push notification. This sets gDirty to true and triggers the getAll.
- The XHR to the server returns, and getAll sets about notifying all listeners.
- The notifications causes the toolbar button to need to update its sate, so it calls getAll.
- getAll is still in the dirty mode (since it hasn't finished updating yet), so this second call causes a second XHR to get all the room details.

We can stop this happening by caching if we've got an XHR in progress, and then using the magic of promises to be resolved at the correct times.
This fixes the issue in my tests. I've had the main patch applied on and off for a while now and haven't noticed problems. I've only just got around to writing the bug and tests ;-)
Attachment #8689484 - Flags: review?(mdeboer)
I've seen this happening too whilst working on the RemotePageManager patches. Great to see this fixed soon!
Status: NEW → ASSIGNED
Iteration: --- → 45.2 - Nov 30
Points: --- → 5
Priority: -- → P2
Comment on attachment 8689484 [details] [diff] [review]
Multiple requests and updates happening when opening a Loop room - make getAll handle multiple requests at a time.

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

I like it! I would like to see one more revision before you land this, though.

::: browser/components/loop/modules/LoopRooms.jsm
@@ +532,4 @@
>     *
>     * @param {String}   [version] If set, we will fetch a list of changed rooms since
>     *                             `version`. Optional.
>     * @param {Function} callback  Function that will be invoked once the operation

Please update the JSDoc here to reflect reality.

@@ +617,5 @@
>        callback(null, room);
>        return;
>      }
>  
> +    MozLoopService.log.error("get");

Hmm, let's remove this before landing, hmkay?

@@ +948,5 @@
>  
>      let oldDirty = gDirty;
>      gDirty = true;
> +
> +    MozLoopService.log.error("onNotification " + version + " " + gDirty);

Same here :)

@@ +1055,5 @@
> +
> +    LoopRoomsInternal.getAll(version).then(result => {
> +      callback(null, result);
> +    }).catch(error => {
> +      callback(error);

nit: this is short enough to be put on a single line. You can omit the braces too for one-line function bodies.

::: browser/components/loop/test/xpcshell/test_looprooms_getall.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Tests can be CC licensed, as in the rest of browser and toolkit test code.
Attachment #8689484 - Flags: review?(mdeboer) → review-
Updated for review comments and to make use of the Task.spawn promise. Also added an additional test to check that errors from getAll were thrown correctly.
Attachment #8689484 - Attachment is obsolete: true
Attachment #8689539 - Flags: review?(mdeboer)
Comment on attachment 8689539 [details] [diff] [review]
Multiple requests and updates happening when opening a Loop room - make getAll handle multiple requests at a time.

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

This is a beauty! Thanks for the unit test.

::: browser/components/loop/modules/LoopRooms.jsm
@@ +537,3 @@
>     */
> +  getAll: function(version = null) {
> +    if (gGetAllPromise && version === null) {

`&& !version) {` should do the trick here. Setting a default param, when there's only one, is a bit moot here.

::: browser/components/loop/test/xpcshell/test_looprooms_getall.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

You forgot to make this CC! See 'test_loopapi_doNotDisturb.js' for an example.

@@ +103,5 @@
> +  yield waitForCondition(() => gQueuedResponses.length == 1);
> +
> +  rejectQueuedResponses();
> +
> +  yield Assert.rejects(firstGetAllPromise, "Not found", "should throw failure");

/me wheeps at seeing the beauty here...
Attachment #8689539 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/dd9778ba38d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.