Closed Bug 994553 Opened 6 years ago Closed 6 years ago

[Sora][Message][Contacts]Contacts's head portrait do not display in Message

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

defect

Tracking

(blocking-b2g:1.4+, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 verified, b2g-v2.1 verified)

RESOLVED FIXED
2.0 S4 (20june)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: sync-1, Assigned: steveck)

References

Details

(Keywords: regression, Whiteboard: [p=2])

Attachments

(5 files)

Firefox OS v1.3
 AU_LINUX_GECKO_B2G_JB_3.2.01.03.00.112.278
 Mozilla build ID:20140323004002
 
 
 DEFECT DESCRIPTION:
 Contacts's head portrait do not display  in Message
 
  REPRODUCING PROCEDURES:
 1.Enter into Message
 2.edit message-->add a contacts(such as mei,10010)-->input content-->send
 3.Enter into contacts-->edit this contact(mei)-->add a picture
 4.Enter into Message,back to thread list view
 5.Contacts's head portrait do not display  in Message, and other's portrait is disappear.
 
  EXPECTED BEHAVIOUR:
 K.O---Contacts's head portrait should display  in Message
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:Medium
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:
 
 It can't be reproduced on v1.1.
Deos the picture appears if you kill and the restart the app?
Flags: needinfo?(sync-1)
Attached file Jrdlog
I don't think it should cause this, but we could check bug 983885.  It changed how we handle canvases after attaching an image.  Maybe we need to reload something in order to handle the activity correctly.
(In reply to Julien Wajsberg [:julienw] from comment #1)
> Deos the picture appears if you kill and the restart the app?

Yes, if you kill the sms app and restart it again, it will be ok.
Flags: needinfo?(sync-1)
I don't reproduce on my peak 1.3.

Reporter, could you please attach a video?
Flags: needinfo?(sync-1)
Switching to qawanted - let's check we can reproduce this on the Moz side on 1.3.
(In reply to Julien Wajsberg [:julienw] from comment #5)
> I don't reproduce on my peak 1.3.
> 
> Reporter, could you please attach a video?

OK, I will give you some video ASAP.
blocking-b2g: --- → 1.3?
ni Tian for video.

QAWanted please check if reproducible
Flags: needinfo?(tianm)
Attached file logcat of issue
Issue is reproducible on 1.3:
Device: Buri 1.3 MOZ
BuildID: 20140414004002
Gaia: 8b92c56267fe50772095f1f25d6cc1f9c9966eb4
Gecko: 3e26908fca71
Version: 28.0
Firmware Version: v1.2-device.cfg

Repro steps:
Pre-requisite:
Have 2 contacts A and B exist in Contacts. A without a portrait picture, B with a portrait picture.

1) Text B a message to create a Message thread
2) Text A a message to create a Message thread
3) Observe within Messages app, that thread A (conversation with A) doesn't have a picture, while thread B does
4) Go to Contacts app, edit A to add a picture
5) Go back to Message app and observe

Expected: What was observed on step 3 should be observed again

Actual: Both thread A and thread B lose their pictures

Attaching a logcat reproducing the issue.
We need a screenshot.
Keywords: qawanted
Attached image screenshots of issue
Adding a combined screenshot demonstrating comment 9 's repro step 3 on the left of the screenshot, and on the right shows what happens on step 5.
This one is found by the carriers during the IOT test, so it is not a IOT blocker
Please, I still really need a video to be able to reproduce, because currently I can't. I understand the issue's symptom but without reproducing it's useless.

Thanks !
(In reply to Julien Wajsberg [:julienw] from comment #13)
> Please, I still really need a video to be able to reproduce, because
> currently I can't. I understand the issue's symptom but without reproducing
> it's useless.
> 
> Thanks !

Here you go~

https://www.youtube.com/watch?v=VWtdHomcY88&feature=youtu.be

Thanks and please let me know if you have any question about reproduce steps
Flags: needinfo?(tianm)
Flags: needinfo?(sync-1)
ok I reproduced the issue. I think this happen only when the thread was not existing before.

I don't think this bug should be a blocker.
reproduce on 1.4 too.
Backlog from 1.3
blocking-b2g: 1.3? → backlog
ok, I got other ways to reproduce. Still not a blocker in my opinion but this looks worse than initially.

For example, it can happen
* when you add/remove an image for a thread that is displayed (no need to send a message)
* when you delete a thread (no need to send a message either)


here is what I think happens:

* we use blob url to display the images
* we revoke the blob url too soon

I removed the "revoke" call, and then it actually works fine.

I think this will happen more when we have a big DOM and reflow takes more time; maybe that's why I reproduce the bug without opening a thread whereas the reporter needs to open the thread before.

So what's the solution here? Obviously we still need to eventually revoke the blob. But maybe it's fine to not revoke it until we either change the src (in the same function, we could store it on a dataset to make it easier to fetch it) or remove the dom node (should be easy to find the places where this happens). Another solution is to schedule the revoke at a very later time (say: setTimeout for 10 seconds).

Steve, what do you think?
Flags: needinfo?(schung)
Vance, can you try the WIP patch in https://github.com/julienw/gaia/compare/mozilla-b2g:v1.4...994553-v14-revoke-later.patch ? Thanks !
Flags: needinfo?(vchen)
(In reply to Julien Wajsberg [:julienw] from comment #19)
> Vance, can you try the WIP patch in
> https://github.com/julienw/gaia/compare/mozilla-b2g:v1.4...994553-v14-revoke-
> later.patch ? Thanks !

This patch can work well.
Hi Julien, the WIP patch works fine. can that be a workaround solution for 1.3 branch?
Flags: needinfo?(vchen)
I need to see exactly where we use this, because this can prevent some memory from being freed. That's why also I'd like to have Steve's advice on this.
This bug is not a blocker anyway, right ?
No it is not a blocker for now, but TCL worries that if other carriers also spot this bug during the IOT test, they might block it since the symptom is quite obvious
(In reply to Julien Wajsberg [:julienw] from comment #18)

> I think this will happen more when we have a big DOM and reflow takes more
> time; maybe that's why I reproduce the bug without opening a thread whereas
> the reporter needs to open the thread before.
> 
> So what's the solution here? Obviously we still need to eventually revoke
> the blob. But maybe it's fine to not revoke it until we either change the
> src (in the same function, we could store it on a dataset to make it easier
> to fetch it) or remove the dom node (should be easy to find the places where
> this happens). Another solution is to schedule the revoke at a very later
> time (say: setTimeout for 10 seconds).
> 
> Steve, what do you think?

I also have this problem in Bug 996516 patch because there is no proper timing to wait for backgroung image url revoke. That's why I didn't revoke it in Bug 996516 at first. Set a timeout is a solution but you can't guarantee that there is no edge case that could block DOM rendering(even we know 10 sec sould be very safe). I think revoke url while removing the dom node should be a reasonalbe solution here and it won't change the code base too much.

BTW, if we want to save the url to dataset, maybe we should also apply visibility monitor lib in shared to clear the url when element out of sight and set it back when visible. It can actually release the image memory, but the drawback is it will affect the scrolling performance. Unless we get serious memory concern, I think it shouldn't be our top priority because of performance issue.
Flags: needinfo?(schung)
See also the thread in dev-b2g https://groups.google.com/forum/#!topic/mozilla.dev.b2g/81lxhZQgevM

It's not obvious to me when some memory is released. It seems to me that the imglib is keeping the blob data around anyway, but I don't know if it's copied or just a reference or...
Duplicate of this bug: 1003789
Duped bug 1003789 is 2.0+. Note that it's not a regression because it exists since 1.3 (likely regression of bug 972245).
Blocks: 972245
blocking-b2g: backlog → 2.0?
Clearing regression keyword per comment 28
Keywords: regression
(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #28)
> Duped bug 1003789 is 2.0+. Note that it's not a regression because it exists
> since 1.3 (likely regression of bug 972245).

No, that still makes a regression. That just means it's a regression from 1.1.
Keywords: regression
triage: regression
blocking-b2g: 2.0? → 2.0+
Blocks: 1010754
No longer blocks: 1010754
Not reproducing the bug in today's master build with hamachi:

B-174
Gecko-564e4e1
Gaia-a436da1
BuildID:20140522070059

Requesting qawanted to retest
Keywords: qawanted
I was able to reproduce this issue on todays Buri Master using Pi Wei's steps from comment 9.  Both conversation's showed no image.

2.0 Environmental Variables:
Device: Buri 2.0 MOZ
BuildID: 20140522062218
Gaia: 7db23414f2d632f4d00b5023ac1090b6045dc5fd
Gecko: 48ef1c52dece
Version: 32.0a1
Firmware Version: v1.2-device.cfg
Maria, it's actually intermittent, and it especially depends on the amount of data you have. The more you have threads, the more it happens.

For example, on my personal dataset, it's not intermittent anymore ;)
Blocks: sms-sprint-3
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S4 (20june)
Assignee: nobody → schung
Attached file Link to github
Hi Julien, this patch makes the url only revoke when element got removed from the dom tree. It should prevent the photo intermittent disappear issue.
Attachment #8438847 - Flags: review?(felash)
Comment on attachment 8438847 [details] [review]
Link to github

One question for you on github
Attachment #8438847 - Flags: review?(felash)
Comment on attachment 8438847 [details] [review]
Link to github

r=me

let's do this, thanks !
Attachment #8438847 - Flags: review+
BTW, I still tried some test per your question. In the medium workflow case, it will take 100~200 ms for the additional mutation observer. Comparing the total threads render time(5sec+), I think it's trivial.
100~200ms is too much already :)
Status: NEW → ASSIGNED
Also, can you file a bug to remove the other uses of Utils.asyncLoadRevokeURL ? I think it's not as urgent as here, but we saw that it can fail intermittently.
(In reply to Julien Wajsberg [:julienw] from comment #40)
> Also, can you file a bug to remove the other uses of
> Utils.asyncLoadRevokeURL ? I think it's not as urgent as here, but we saw
> that it can fail intermittently.

Bug created in Bug 1025737 :)
Travis is green but some failed on gaia try... Seems some issue in linux desktop 64.

In master: 5f9f64a05c15145df6ed33a7807adcebb11931f3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1023256
duped bug 1023256 is 1.4+, please set this one to 1.4+ and uplift.
blocking-b2g: 2.0+ → 1.4?
Flags: needinfo?(bbajaj)
(In reply to Julien Wajsberg [:julienw] from comment #45)
> duped bug 1023256 is 1.4+, please set this one to 1.4+ and uplift.

Blocking as the DUP was deemed a blocker and we are targeting the real fix here.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(bbajaj)
Attached video Verify_video.3gp
This issue has been verified successfully on Flame 2.0,2.1

See attachment: Verify_video.3gp
Reproducing rate: 0/5
Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
FLame 2.0 build:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0
You need to log in before you can comment on or make changes to this bug.