959285 - [Search] Abort in-progress provider requests

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

Currently it's possible to have stale requests complete unexpectedly. We should abort any in-progress XHRs, potentially in the clear() method.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8359376 [details] [review]
Github pull request

Hi Ran, Amir - Wondering if either one of you could review this. Basically this just aborts any in-progress requests. This is a slight optimization, but also ensures that we never show stale results.

Comment 3

[Amir Nissim (:amirn)]

Added some comments on Github.
Thanks Kevin.

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (Everything.me) from comment #3)
> Added some comments on Github.
> Thanks Kevin.

Thanks Amir. I updated the pull request. I'm not sure that calling reject is necessary as if we abort the in-progress XHR, the success handler should never be called? Please take another look and let me know what you think.

Comment 5

[Amir Nissim (:amirn)]

(In reply to Kevin Grandon :kgrandon from comment #4)
> I'm not sure that calling reject is necessary as if we abort the in-progress XHR, the success handler should
> never be called? Please take another look and let me know what you think.

I think it is possible for Search to call abort when the request is not abortable.
In which case we would still like to abort the success callback, regardless of the state of the httpRequest.

Comment 6

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (Everything.me) from comment #5)
> I think it is possible for Search to call abort when the request is not
> abortable.
> In which case we would still like to abort the success callback, regardless
> of the state of the httpRequest.

I still don't understand. There should be no harm if the search app calls abort on an old XHR object. In this case it would've already fired it's success handler. Would you happen to have the time to write up some code perhaps and show me if possible?

Comment 7

[Amir Nissim (:amirn)]

Maybe I am wrong, but I think theoretically the following scenario can happen.

lines 1,2: timeframe of search requests
line    3: rendered results on screen (1 are request1's results, etc)

--input1--request1--send1---load1--render1-------------------------------
--------------------------input2--abort1--request2--send2--load2--render2
------------------------------------------1111111111111111111111112222222

where the desired behavior would be:
------------------------------------------------------------------2222222

However, the solution I suggested is not possible (see my comment on Github).
So I suggest we tackle this in another bug - if and when relevant.

Comment 8

[Amir Nissim (:amirn)]

Comment on attachment 8359376 [details] [review]
Github pull request

Small comment on Github. Need to revert to previous version.

Thanks.

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8359376 [details] [review]
Github pull request

Hi Amir,

Thank you for the review. I don't believe that the proposed situation is possible due to the abort cancelling the success handler, and thus the rendering.

If it is though - let's certainly follow this up with a patch, or even back this one out. Thanks!

Comment 10

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/bcf6fb4859b559ecfea884d6a7646685d72b9fd6