[SMS] Resend handler is bound unsafely

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jugglinmike, Assigned: jugglinmike)

Tracking

unspecified
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Currently, a unique "resend" handler is bound to each message element individually on an as-needed basis. This approach is prone to runtime bugs such as multiple handlers being bound at once, or handlers not being removed when the message state changes.

To avoid bugs like these (and to make the code more performant/maintainable), re-factor the "resend" handler to be bound to the Thread's container element and delegate to each message.
(Assignee)

Updated

6 years ago
Depends on: 840055
blocking-b2g: --- → leo+
Whiteboard: [NO_UPLIFT]

Updated

5 years ago
Assignee: nobody → mike
(Assignee)

Comment 1

5 years ago
Making a note here that while the "resend" handler is currently bound to the top-level message element, this is not desirable. The message element spans the entire width of the application, which can be surprising for a user who is attempting to scroll through the thread.

The patch for this bug should instead bind the "resend" handler to the message "bubble" element whose boundaries are well-defined in the UI.
(Assignee)

Comment 2

5 years ago
Created attachment 750085 [details] [diff] [review]
Use event delegation for message resend handler
Attachment #750085 - Flags: review?(gnarf37)

Comment 3

5 years ago
Comment on attachment 750085 [details] [diff] [review]
Use event delegation for message resend handler

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

quick stuff here I think

message_manager_tst.js should be message_manager_test.js

::: apps/sms/js/thread_ui.js
@@ +123,5 @@
>      );
>  
>      // Delegate to |this.handleEvent|
>      this.container.addEventListener(
> +      'click', this.handleEvent.bind(this)

Let's bind the rest of these as well

@@ +981,5 @@
>    handleEvent: function thui_handleEvent(evt) {
>      switch (evt.type) {
>        case 'click':
>          if (window.location.hash !== '#edit') {
> +          this.handleMessageClick(evt);

There are a few refs to ThreadUI below, lets convert them to this

@@ +1183,5 @@
> +        removeFromDOM();
> +        // We resend again
> +        this.sendMessage(message.body);
> +      }.bind(this));
> +    }.bind(this);

Lint complains about this, which is a shame, but if you put () around the function it works fine.
(Assignee)

Comment 4

5 years ago
Created attachment 750116 [details] [diff] [review]
Use event delegation for message resend handler

This version implements Corey's feedback in comment 3. Namely:

- Fix a linting error
- More consistent binding of event handlers
Attachment #750085 - Attachment is obsolete: true
Attachment #750085 - Flags: review?(gnarf37)
Attachment #750116 - Flags: review?(gnarf37)

Updated

5 years ago
Attachment #750116 - Flags: review?(gnarf37) → review+

Comment 5

5 years ago
master:9cce95ca788f653e8915c71d41c81f831edcd62c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 875396

Updated

5 years ago
Whiteboard: [NO_UPLIFT]
v1-train: 5c95d71
status-b2g18: --- → fixed

Comment 7

5 years ago
Can you please provide steps to verify this fix - as we can perform blackbox testing from the UI?

Comment 8

5 years ago
The fact that you can still resend is all that should be tested here.  The bug was basically improving the method to bind the event
You need to log in before you can comment on or make changes to this bug.