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

ASSIGNED
Assigned to

Status

()

Toolkit
General
P3
normal
ASSIGNED
2 years ago
2 years ago

People

(Reporter: tracy, Assigned: adw)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
Blocks: 1245225
tracking-e10s: --- → +
Priority: -- → P3
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?).
Flags: needinfo?(adw)
(Assignee)

Comment 2

2 years ago
Sure.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
(Assignee)

Comment 3

2 years ago
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.

Review commit: https://reviewboard.mozilla.org/r/52323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52323/
Attachment #8751963 - Flags: review?(dao+bmo)
(Assignee)

Updated

2 years ago
Blocks: 1088203
(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?
Flags: needinfo?(adw)

Updated

2 years ago
Attachment #8751963 - Flags: review?(dao+bmo)
(Assignee)

Comment 5

2 years ago
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.
Flags: needinfo?(adw)
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.
Attachment #8751963 - Flags: review?(jmathies)

Comment 7

2 years ago
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.
Attachment #8751963 - Flags: review?(jmathies)

Comment 8

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.