Closed Bug 1096507 Opened 10 years ago Closed 6 years ago

[Wallpaper] Repeatedly tapping gallery, the screen does not load up images/blank

Categories

(Firefox OS Graveyard :: Gaia::Wallpaper, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.0 unaffected, b2g-v2.1 unaffected, b2g-v2.2 affected)

RESOLVED WONTFIX
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- affected

People

(Reporter: psiphantong, Unassigned)

References

()

Details

(Keywords: regression, Whiteboard: [2.1-exploratory-3])

Attachments

(2 files)

Attached file dg.txt
Description:
Repeatably tapping the gallery button, the screen does not load up images/blank

Setup Steps:
1) Flame device is set to 319mb

Repro Steps:
1) Update a Flame device to BuildID: 20141110040206
2) Go to Settings 
3) Repeatably tap the wallpaper button
4) Repeatably tap the gallery button


Actual:
screen does not load up images/blank

Expected:
images load

Flame 2.2

Device: Flame 2.2 Master(319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110040206
Gaia: 5f8206bab97cdd7b547cc2c8953cadb2a80a7e11
Gecko: d380166816dd
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0


Repro frequency: 100%
See attached: video, logcat, https://www.youtube.com/watch?v=SUe_JJLtMFo
This issue does not reproduce on the Flame 2.1 and the Flame 2.0, images load correctly.

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.0

Device: Flame 2.0 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110000204
Gaia: d3e4da377ee448f9c25f908159480e867dfb13f3
Gecko: 7198906837e7
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Summary: [Wallpaper] Repeatably tapping gallery, the screen does not load up images/blank → [Wallpaper] Repeatedly tapping gallery, the screen does not load up images/blank
NI to Settings owner to make a blocking call - On one hand this is a regression and brings the user to a blank page / gallery but on the other hand this is an edge case with a user tapping 'gallery' multiple times quickly
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell) → needinfo?(gchang)
Hi Arthur,
This might need your help to investigate.
Flags: needinfo?(gchang) → needinfo?(arthur.chen)
I could not reproduce the bug on master. And from this line of code[1] the activity menu is opened only once even if we create multiple moz activities. I think we should check this issue from the gallery side.

Justin, could you help on it? Thanks.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/activities.js#L100
Flags: needinfo?(arthur.chen) → needinfo?(jdarcangelo)
(In reply to Arthur Chen [:arthurcc] from comment #4)
> I could not reproduce the bug on master. And from this line of code[1] the
> activity menu is opened only once even if we create multiple moz activities.
> I think we should check this issue from the gallery side.
> 
> Justin, could you help on it? Thanks.
> 
> [1]:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/activities.
> js#L100

This revised STR indicates the root cause of this issue is coming from Settings and not Gallery. I was definitely able to reproduce on master with a 100% rate using these steps:

1. Launch "Settings" app
2. Select "Display" menu
3. Repeatedly tap the wallpaper button
4. Select "Gallery" (doesn't matter how many times you tap it)

Somehow repeatedly tapping the wallpaper button is causing this issue and it doesn't seem to have anything to do with how many times you tap "Gallery". There is a console warning being logged from Gallery though:

W/Gallery ( 7569): Content JS WARN: MediaDB:  onready event handler threw Error: MediaDB is not ready. State: opening enumerate@app://gallery.gaiamobile.org/shared/js/mediadb.js:1132:1

It seems to me that its possible that the Gallery app is receiving the activity multiple times and that's causing us to get into this state. Investigating further to see if Gallery is actually receiving the activity multiple times.
Ok, so my assumptions were correct. Gallery is receiving multiple "pick" activities if the "wallpaper" button is tapped repeatedly in the Settings app. That's causing a failure around this point in the Gallery code:

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/gallery.js#L215

```
    case 'pick':
      picking = true;
      initDB();
      LazyLoader.load('js/pick.js', function() { Pick.start(a); });
      break;
```

The `initDB()` call is being made multiple times in the same instance of Gallery which is causing a failure. So, there are two things here that I think need to be addressed:

1.) Gallery's `initDB()` function needs to be a little more robust to handle scenarios where it gets called multiple times (even though that shouldn't happen).

2.) The Settings app needs to disable the "wallpaper" button immediately after it is tapped to prevent this kind of hammering.

Assigning to myself.
Assignee: nobody → jdarcangelo
Flags: needinfo?(jdarcangelo)
Attached file pull-request (master)
David: Can you please review this? This patch addresses the problem from both ends. It hardens Gallery to be more robust to protect against scenarios where multiple pick activities are fired repeatedly (even though that *shouldn't* happen) and it also fixes the issue by disabling the Settings app from triggering a new "pick" activity while another activity is already pending.

If you're not an eligible reviewer for the Settings part of this patch, please re-assign the review to someone who is (not sure who that should be). Thanks!
Attachment #8523056 - Flags: review?(dflanagan)
Attachment #8523056 - Flags: review?(21)
(In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> Created attachment 8523056 [details] [review]
> pull-request (master)
> 
> David: Can you please review this? This patch addresses the problem from
> both ends. It hardens Gallery to be more robust to protect against scenarios
> where multiple pick activities are fired repeatedly (even though that
> *shouldn't* happen) and it also fixes the issue by disabling the Settings
> app from triggering a new "pick" activity while another activity is already
> pending.
> 
> If you're not an eligible reviewer for the Settings part of this patch,
> please re-assign the review to someone who is (not sure who that should be).
> Thanks!

BTW, I saw Vivien was a module peer for Settings so I also set the R? for him as well. I'm not sure if he's active on Settings currently or not though.
Comment on attachment 8523056 [details] [review]
pull-request (master)

r+ for the gallery part.

I've left comments on the settings part, but am unfamiliar enough with the settings part that I'm not comfortable formally reviewing that.  If you add tests for your changes to apps/settings/test/unit/panels/display/wallpaper_test.js it will probably make the review go more smoothly.

I've heard that Vivien is OOO, so I'm changing the settings review request to Arthur
Attachment #8523056 - Flags: review?(dflanagan)
Attachment #8523056 - Flags: review?(arthur.chen)
Attachment #8523056 - Flags: review?(21)
Attachment #8523056 - Flags: review+
Comment on attachment 8523056 [details] [review]
pull-request (master)

Thanks for the patch! I think we can make the patch simpler, please check my comments in github.
Attachment #8523056 - Flags: review?(arthur.chen)
Comment on attachment 8523056 [details] [review]
pull-request (master)

Arthur, I've updated the PR to use promises as you suggested. Although, I'm not 100% certain that the pattern I used is what you were expecting. I looked through some of the rest of the Settings app and seen similar things so I think its right. Could you please give feedback and if it looks ok, I'll go ahead and add/update tests before flagging you for review again. Thanks!
Attachment #8523056 - Flags: feedback?(arthur.chen)
Comment on attachment 8523056 [details] [review]
pull-request (master)

The pattern you are using is for "retrieving" things but which is not the case here. Both "_triggerActivity" and "selectWallpaper" are more like commands.

In your previous patch using a flag is totally fine. The reason I suggested to return a promise is that we can then be able to write tests for cases of calling to "selectWallpaper" repeatedly or calling after the previous call to it resolved.

In summary, we don't need to cache the promises and use a simple flag instead. If the selection in still in progress and there is another call to "selectWallpaper", reject it. BTW, now "selectWallpaper" may reject, so we have to catch it in js/panels/display/panel.js.
Attachment #8523056 - Flags: feedback?(arthur.chen)
Comment on attachment 8523056 [details] [review]
pull-request (master)

Arthur, ok I think I understand what you're saying now. I've gone back to using a boolean flag and now both `_triggerActivity` and `selectWallpaper` return promises. The promise returned from `_triggerActivity` is either resolved or rejected when the activity succeeds or fails. When `selectWallpaper` calls `_triggerActivity`, it uses the promise received to determine when to clear the `_selectingWallpaper` flag. Additionally, the promise returned from `selectWallpaper` will automatically be rejected if the `_selectingWallpaper` flag is set at the time of the call (this stops subsequent calls from triggering additional activities until the current activity is complete). Let me know how the PR looks now. Thanks!
Attachment #8523056 - Flags: review?(arthur.chen)
Comment on attachment 8523056 [details] [review]
pull-request (master)

The code looks good! However, note that now "selectWallpaper" may reject, so we have to catch it in js/panels/display/panel.js or it leads to js error.

Please also add new tests in "wallpaper_test.js", thanks!
Attachment #8523056 - Flags: review?(arthur.chen) → feedback+
(In reply to Arthur Chen [:arthurcc] from comment #14)
> Comment on attachment 8523056 [details] [review]
> pull-request (master)
> 
> The code looks good! However, note that now "selectWallpaper" may reject, so
> we have to catch it in js/panels/display/panel.js or it leads to js error.
> 
> Please also add new tests in "wallpaper_test.js", thanks!

`selectWallpaper` is simply called as an event handler in display/panel.js, so it fails silently if it rejects which is correct. I'll go ahead and update the tests now. Thanks!
See Also: → 1088418
Not working on this. Unassigning myself.
Assignee: jdarcangelo → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: