Closed Bug 975217 Opened 6 years ago Closed 6 years ago

Don't try to remove a banner message that doesn't exist

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: Margaret, Assigned: Margaret)

Details

Attachments

(1 file)

Currently, if you pass a bad id to Home.banner.remove, it just removes the last message in the rotation. That's not good.
Attachment #8379373 - Flags: review?(michael.l.comella)
Comment on attachment 8379373 [details] [diff] [review]
bannerremove.diff

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

(In reply to :Margaret Leibovic from comment #0)
> Currently, if you pass a bad id to Home.banner.remove, it just removes the
> last message in the rotation. That's not good.

I'm not sure I see that - it would either delete nothing (i.e. invalid ID) or delete something it shouldn't (i.e. wrong ID).

But the validation check is a good idea in any case.
Attachment #8379373 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #1)
> Comment on attachment 8379373 [details] [diff] [review]
> bannerremove.diff
> 
> Review of attachment 8379373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Margaret Leibovic from comment #0)
> > Currently, if you pass a bad id to Home.banner.remove, it just removes the
> > last message in the rotation. That's not good.
> 
> I'm not sure I see that - it would either delete nothing (i.e. invalid ID)
> or delete something it shouldn't (i.e. wrong ID).
> 
> But the validation check is a good idea in any case.

If we have an invalid id, we end up calling _queue.splice(-1, 1) here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Home.jsm#142

If the index passed to splice is negative, it will begin that many elements from the end:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice#Parameters

That's the problem I noticed when testing locally.
https://hg.mozilla.org/mozilla-central/rev/fb9c889df184
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.