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

Status: RESOLVED FIXED

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

Description

[sync-1]

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.

Comment 1

[Julien Wajsberg [:julienw]]

Deos the picture appears if you kill and the restart the app?

Comment 2

[sync-1]

Attachment: Jrdlog

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[buri.blff]

(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.

Comment 5

[Julien Wajsberg [:julienw]]

I don't reproduce on my peak 1.3.

Reporter, could you please attach a video?

Comment 6

[Jason Smith [:jsmith]]

Switching to qawanted - let's check we can reproduce this on the Moz side on 1.3.

Comment 7

[田旻]

(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.

Comment 8

[Preeti Raghunath(:Preeti)]

ni Tian for video.

QAWanted please check if reproducible

Comment 9

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: 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.

Comment 10

[Jason Smith [:jsmith]]

We need a screenshot.

Comment 11

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: 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.

Comment 12

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

This one is found by the carriers during the IOT test, so it is not a IOT blocker

Comment 13

[Julien Wajsberg [:julienw]]

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 !

Comment 14

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 15

[Julien Wajsberg [:julienw]]

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.

Comment 16

[Julien Wajsberg [:julienw]]

reproduce on 1.4 too.

Comment 17

[Preeti Raghunath(:Preeti)]

Backlog from 1.3

Comment 18

[Julien Wajsberg [:julienw]]

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?

Comment 19

[Julien Wajsberg [:julienw]]

Vance, can you try the WIP patch in https://github.com/julienw/gaia/compare/mozilla-b2g:v1.4...994553-v14-revoke-later.patch ? Thanks !

Comment 20

[田旻]

(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.

Comment 21

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Julien, the WIP patch works fine. can that be a workaround solution for 1.3 branch?

Comment 22

[Julien Wajsberg [:julienw]]

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.

Comment 23

[Julien Wajsberg [:julienw]]

This bug is not a blocker anyway, right ?

Comment 24

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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

Comment 25

[Steve Chung [:steveck]]

(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.

Comment 26

[Julien Wajsberg [:julienw]]

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...

Comment 28

[Julien Wajsberg [:julienw]]

Duped bug 1003789 is 2.0+. Note that it's not a regression because it exists since 1.3 (likely regression of bug 972245).

Comment 29

[Noemí Freire (:noemi)]

Clearing regression keyword per comment 28

Comment 30

[Jason Smith [:jsmith]]

(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.

Comment 31

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression

Comment 32

[Maria Angeles Oteo (:oteo)]

Not reproducing the bug in today's master build with hamachi:

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

Requesting qawanted to retest

Comment 33

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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

Comment 34

[Julien Wajsberg [:julienw]]

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 ;)

Comment 35

[Steve Chung [:steveck]]

Attachment: 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.

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 8438847 [details] [review]
Link to github

One question for you on github

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8438847 [details] [review]
Link to github

r=me

let's do this, thanks !

Comment 38

[Steve Chung [:steveck]]

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.

Comment 39

[Julien Wajsberg [:julienw]]

100~200ms is too much already :)

Comment 40

[Julien Wajsberg [:julienw]]

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.

Comment 41

[Steve Chung [:steveck]]

(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 :)

Comment 42

[Steve Chung [:steveck]]

Travis is green but some failed on gaia try... Seems some issue in linux desktop 64.

In master: 5f9f64a05c15145df6ed33a7807adcebb11931f3

Comment 43

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/9f2753e3ee43bfbe0a1cf03c37db2a56b7d2b735

Comment 45

[Julien Wajsberg [:julienw]]

duped bug 1023256 is 1.4+, please set this one to 1.4+ and uplift.

Comment 46

[bhavana bajaj [:bajaj]]

(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.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/b7a059d6f9cfcf56ea461b43a1b2e1892b04a398

Comment 48

[Paladin]

Attachment: 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