1245118 - [e10s] Thumbnail service must not overwrite existing thumbnails if it gets an error response.

Status: ASSIGNED

(Toolkit :: General, defect, P3)

Description

[Tracy Walker [:tracy]]

I discovered this bug while investigating the test toolkit/components/thumbnails/test/browser_thumbnails_updates.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/test/browser_thumbnails_update.js).  capIfStaleErrorResponseUpdateTest() and regularCapErrorResponseUpdateTest() are both failing when the server returns a 400 response and a red thumbnail. The green thumbnail should not be updated to red in each test.

This works fine in non-e10s, but is failing in e10s.  (this is essentially an e10s version of bug 897880)

Comment 1

[Justin Dolske [:Dolske]]

Drew - is this something you might be able to look at? This is apparently blocking enabling one of the last few front-end tests for E10S (and sounds like this might be an actual bug?).

Comment 2

[Drew Willcoxon :adw]

Sure.

Comment 3

[Drew Willcoxon :adw]

Attachment: MozReview Request: Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for e10s. r?dao

Bug 1088203 broke this test (and the PageThumbs API, IMO) in e10s.

Review commit: https://reviewboard.mozilla.org/r/52323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52323/

Comment 4

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #3)
> Created attachment 8751963 [details]
> MozReview Request: Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for
> e10s. r?dao
> 
> Bug 1088203 broke this test (and the PageThumbs API, IMO) in e10s.

Shouldn't we fix the API then? How exactly is it broken?

Comment 5

[Drew Willcoxon :adw]

In short it's broken because captureAndStore is a public method, but to use it safely you have to know to call shouldStoreThumbnail first.  I'd have to think about it more but I can't really prioritize fixing it because it's not one of my deliverables, so I think we should just fix this bug and take the patch.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8751963 [details]
MozReview Request: Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for e10s. r?dao

I think I'd prefer Jim to take a look at this. Let me know if he's too busy, although since this is e10s-related I suppose it's actually within his current area of focus.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8751963 [details]
MozReview Request: Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for e10s. r?dao

https://reviewboard.mozilla.org/r/52323/#review53148

::: toolkit/components/thumbnails/PageThumbs.jsm:381
(Diff revision 1)
>      } else {
> -      // We need channel info (bug 1073957)
> +      // In this case the thumbnail will be unconditionally stored, and any
> +      // existing thumbnail will be overwritten.  If that's not what you want,
> +      // you should call shouldStoreThumbnail before calling this method.
>        originalURL = url;
>      }

AFAICT this check code for both local and remote tabs can be combined into a single async call to shouldStoreThumbnail. This should be pretty straight forward since we're already working inside an async api and there's a task available just below here.

There's a related e10s bug here - originalURL should contain the url that was loaded prior to an about page loading in the tab (if there was one). Lets split this out to a new bug.. no harm can come from it, we might end up with an about page with a thumbnail is all.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #7)
> There's a related e10s bug here - originalURL should contain the url that
> was loaded prior to an about page loading in the tab (if there was one).
> Lets split this out to a new bug.. no harm can come from it, we might end up
> with an about page with a thumbnail is all.

This might not be the case actually, url may contain the right value under e10s. We don't want to regress non-e10s though so we should keep the originalURL bits here for non-remote browsers.