949470 - [Contacts] Broken image links when entering the search mode

Status: RESOLVED FIXED

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

Description

[Germán Toro del Valle (:gtorodelvalle)]

TO REPRODUCE:
1. Open the contacts app and include (or scroll to) some contacts with photos as shown in attachment 1 [details] [diff] [review]. Having only 1 contact in view is enough.
2. Enter the "search mode" by clicking on the search bar.

OBSERVED:
3. The contacts' photo links are broken as shown in attachment 2 [details] [diff] [review].

EXPECTED:
3. The contacts' photos are shown.

Comment 1

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: attachment 1

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: attachment 2

Comment 3

[Germán Toro del Valle (:gtorodelvalle)]

As far as my analysis, the problem is due to:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L832
in conjunction with the code which initializes the ImageLoader when entering the "search mode", and more concretely the one which checks if list item where the image is included has already been visited, this is:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/image_loader.js#L122

A possible and straightforward solution to the problem would be to include the following line:
img.parentNode.parentNode.dataset.visited = false;
after https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L832 but, apart from the ugly code :-) , since this may cause other regressions (mainly regarding performance) and there is probably a more efficient solution I would like to ask Ben about this issue to know his opinion about it :-) Thanks!

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

BTW, I found this issue, well in fact it was José Manuel :-p , using Gecko-f88ec24 and latest Gaia... Noemí is checking if it also reproduces in 1.3 right now :-)

Comment 5

[Noemí Freire (:noemi)]

(In reply to gtorodelvalle from comment #4)
> BTW, I found this issue, well in fact it was José Manuel :-p , using
> Gecko-f88ec24 and latest Gaia... Noemí is checking if it also reproduces in
> 1.3 right now :-)

Hi,

It also occurs with today's (12/12) build in v1.3 branch so nominating to v1.3?:
gaia: cbd2921
gecko: c8ebb7f
BuildId: 20131212024136
Platform version: 28.0a2

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: contact-search-img.patch

Can you try this untested patch?  I think the issue is that when we clone the DOM node from the main list to the search list they end up sharing the same URL.  Then when one the URL is revoked a stale reference is left in the other list.

This patch tries to produce a separate URL in the search list.

(Side issue, do we ever actually revoke the URLs in the nodes in the search list??  Sounds like a possible memory issue there.)

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Hi Ben, the proposed patch solves the issue ;-)

On the other hand and regarding the revoking of URLs in the nodes when in search mode and according to the observed behaviour, I would say we do revoke URLs :-)

Would you send a PR with the proposed patch or would you like me to do it? I would ask José Manuel for revision to be completely sure about the issue. Thanks!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to gtorodelvalle from comment #7)
> Would you send a PR with the proposed patch or would you like me to do it? I
> would ask José Manuel for revision to be completely sure about the issue.
> Thanks!

I would be grateful if you could write the patch.  I'm a bit buried investigating perf regressions at the moment.  Thanks!

Comment 9

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 14806.html

PR sent on behalf of Ben according to the patch proposed by him ;-)

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ regression

Comment 11

[Jose Manuel Cantera]

Comment on attachment 8349357 [details]
14806.html

works ok, pleae check Travis before landing

thanks

Comment 12

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/adba622b1072a94426ba618fb3e407e4393461df

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted adba622b1072a94426ba618fb3e407e4393461df to:
v1.3: d4f7cb3bb4bec8f28f1efedae99d638c1d003998