New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic when updating a non-tiled "multi" image cache entry. #3354

Merged
merged 1 commit into from Nov 26, 2018

Conversation

Projects
None yet
3 participants
@nical
Collaborator

nical commented Nov 26, 2018

In #3277 I introduced the expectation that if a cached image entry is ImageResult::Multi(..) then it must be tiled and we can unwrap an optional tiling related variable.

It turns out (from bug 1509643) that this assumption doesn't always hold true. This change restores the previous behavior and also invalidates the entire entry if it is tiled but we don't have information about the tile size. I don't know for sure whether this case can happen in practice but I'd rather be conservative about it since invalidation bugs tend to be hard to track down.


This change is Reviewable

@nical

This comment has been minimized.

Collaborator

nical commented Nov 26, 2018

@nical

This comment has been minimized.

@aosmond

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@nical

This comment has been minimized.

Collaborator

nical commented Nov 26, 2018

@bors-servo r=aosmond

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

📌 Commit 76cfa92 has been approved by aosmond

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

⌛️ Testing commit 76cfa92 with merge 05bdcae...

bors-servo added a commit that referenced this pull request Nov 26, 2018

Auto merge of #3354 - nical:tiling-panic, r=aosmond
Fix panic when updating a non-tiled "multi" image cache entry.

In  #3277 I introduced the expectation that if a cached image entry is `ImageResult::Multi(..)` then it must be tiled and we can `unwrap` an optional tiling related variable.

It turns out (from [bug 1509643](https://bugzilla.mozilla.org/show_bug.cgi?id=1509643)) that this assumption doesn't always hold true. This change restores the previous behavior and also invalidates the entire entry if it is tiled but we don't have information about the tile size. I don't know for sure whether this case can happen in practice but I'd rather be conservative about it since invalidation bugs tend to be hard to track down.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3354)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: aosmond
Pushing 05bdcae to master...

@bors-servo bors-servo merged commit 76cfa92 into servo:master Nov 26, 2018

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 1 file reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 27, 2018

WR Updater Bot
Bug 1510085 - Update webrender to commit 05bdcae134d73aca7bb48358e91d…
…e1f8aef27773 (WR PR #3354). r=kats

servo/webrender#3354

Differential Revision: https://phabricator.services.mozilla.com/D13029

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment