This image https://ssl.gstatic.com/gb/images/v2_ad17f477.png stops working. This is persistent across reloads. If I load this image in a new tab it doesn't show up when scaled at 100% or larger. If I zoom out to 30% it does show up. If I load it in private browsing it does show up.

This is probably a regression because it didn't happen before and seems unrelated to site behaviour.

Shift reloading the image fixes the problem.

This happened to me again.

Assuming not running with stylo.

I'm running with stylo now, but I saw it before I started running with stylo.

Technically imagelib use of VolatileBuffer relies upon SurfaceCache being the first thing to lock it so that it gets the expected purge state information. This is bad because if something else maps the surface in first and sees the bad state, we would forget it was purged. VolatileBuffer should probably remember that and force the owner to clear that flag explicitly once it has repopulated the data. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5381c46a5e3b6219d47bfb19bc5835a8b1400560 Note that this is speculative, no evidence yet that this will fix it :).

Remove VolatileBufferPtr::mPurged, forgot about that and it broke all the try builds...sigh. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c913db020ba95e12821cdd87cdfb562ddede90d

I didn't do the right thing on OSX, hence the gtest failure... try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b68145799a745128062d5a4d047c106f40193c7f

I just got this on facebook.com

Comment on attachment 8896293 [details] [diff] [review] VolatileBuffer should remember if previously purged and the owner should explicitly clear that state., v3 Review of attachment 8896293 [details] [diff] [review]: ----------------------------------------------------------------- Can you walk me through what this change does? because it seems idempotent to me, except for exposing a ClearBufferPurged, which I'm not entirely sure has its place here or is useful. I also don't see how it's related to the bug this is attached to. Specifically, can you say why you think mPurged should be attached to the buffer instead of the ptr? To me, the ptr class can be thought as a tuple, where you get the ptr and whether it points to purged memory or not, and from there the called does what it wants to. I can however see a problem with VolatileBuffer, but I don't see it addressed in this patch: when two ptr instances are created on the same buffer, and one is destroyed, it unlocks the buffer, meaning the OS is free to drop the corresponding pages, while the user of the first ptr still thinks it's fine. But I don't remember the class having been designed for this use case.

(In reply to Mike Hommey [:glandium] from comment #10) > Comment on attachment 8896293 [details] [diff] [review] > VolatileBuffer should remember if previously purged and the owner should > explicitly clear that state., v3 > > Review of attachment 8896293 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you walk me through what this change does? because it seems idempotent > to me, except for exposing a ClearBufferPurged, which I'm not entirely sure > has its place here or is useful. I also don't see how it's related to the > bug this is attached to. > I poked around the memory reports with Jeff when this happened to him. What it showed was the image was still in the surface cache, with multiple surfaces decoded at different sizes. From the display, I determined that the largest surface was somehow zero filled despite displaying correctly before suspend, and the smaller surfaces contained the expected data. I have not been able to reproduce myself, but my hunch is that the gmail tab was in the background, meaning its images would be unlocked (i.e. no VolatileBufferPtr is alive) and thus open to purging by the OS. On suspend or reboot the OS took advantage of this for some pages, and thus our reclaimed pages when we did create a VolatileBufferPtr were zero filled. This would be consistent with the transparent display. If the surface cache in imagelib is the first to try to acquire the volatile buffer, then it will catch this condition and redecode the image at the desired size. It did not do that. My guess is something else acquired it first, saw that it was purged and failed its operation. To be clear, I don't know for certain if this will work based on how reproducible it is. But it is my best guess right now. > Specifically, can you say why you think mPurged should be attached to the > buffer instead of the ptr? To me, the ptr class can be thought as a tuple, > where you get the ptr and whether it points to purged memory or not, and > from there the called does what it wants to. > If the surface is passed around, then the end consumer of the buffer might not have anything to do with the producer (imagelib) and thus have no idea or interest in figuring out how to restore it (redecode the image). This shouldn't prevent the producer from finding out later that the data was purged and doing the restoration itself. (Not only that, imagelib itself doesn't even repopulate the buffer after finding out it is purged -- it discards it and it later on reallocates a new buffer when it is redecoding.) > I can however see a problem with VolatileBuffer, but I don't see it > addressed in this patch: when two ptr instances are created on the same > buffer, and one is destroyed, it unlocks the buffer, meaning the OS is free > to drop the corresponding pages, while the user of the first ptr still > thinks it's fine. But I don't remember the class having been designed for > this use case. Each platform variant of VolatileBuffer::Lock/Unlock maintains a counter of how many consumers there are of the buffer. imagelib has always been using it in this manner -- the decoders are writing into the buffer, as readers are drawing it (e.g. this effect is visible with large images where you see it draw line-by-line as it decodes). Am I missing something?

(In reply to Andrew Osmond [:aosmond] (PTO 08/19-08/28) from comment #11) > If the surface is passed around, then the end consumer of the buffer might > not have anything to do with the producer (imagelib) and thus have no idea > or interest in figuring out how to restore it (redecode the image). This > shouldn't prevent the producer from finding out later that the data was > purged and doing the restoration itself. I'm sorry but I don't see how your patch makes things better for that case. Maybe it's missing changes in the consumer to make sense to me. > Each platform variant of VolatileBuffer::Lock/Unlock maintains a counter of > how many consumers there are of the buffer. That's true. Which begs the question: what is it you think is going wrong? And how does your patch solve it? I'm afraid I don't see it. Not knowing how imagelib is actually using this doesn't help too, and from your description, it would seem the problem would be on the consumer end, not in VolatileBuffer, but again, I don't know what the consumer is doing. So it would really help if you could detail your thoughts about the series of events/functions calls that you think end up making images blank.

Based on priority and the fact that we're close to merging 57.

(In reply to Mike Hommey [:glandium] from comment #12) > (In reply to Andrew Osmond [:aosmond] (PTO 08/19-08/28) from comment #11) > > If the surface is passed around, then the end consumer of the buffer might > > not have anything to do with the producer (imagelib) and thus have no idea > > or interest in figuring out how to restore it (redecode the image). This > > shouldn't prevent the producer from finding out later that the data was > > purged and doing the restoration itself. > > I'm sorry but I don't see how your patch makes things better for that case. > Maybe it's missing changes in the consumer to make sense to me. > > > Each platform variant of VolatileBuffer::Lock/Unlock maintains a counter of > > how many consumers there are of the buffer. > > That's true. Which begs the question: what is it you think is going wrong? > And how does your patch solve it? I'm afraid I don't see it. Not knowing how > imagelib is actually using this doesn't help too, and from your description, > it would seem the problem would be on the consumer end, not in > VolatileBuffer, but again, I don't know what the consumer is doing. > > So it would really help if you could detail your thoughts about the series > of events/functions calls that you think end up making images blank. Sorry for the delay on the response. After some thought, you are right. I'm putting the cart before the horse. I should land a diagnostic patch first to confirm the hunch rather than try to fix it and see if that works :). The attached patch adds a diagnostic assert for when we hit the suspected condition. The stack trace will show us the culprit, if we indeed trip it. I can actually do this inside the SourceSurfaceVolatileData object wrapping the VolatileBuffer. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=764d7ba829308186a6ccb7458e6468f4b8f1b11a

(In reply to Jim Mathies [:jimm] from comment #13) > Based on priority and the fact that we're close to merging 57. I'd like to understand how common this is before we do this. This is a type of report that we'd rarely get, so I'd have to guess a high multiplier value for it.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/722569a89186 Assert if we remap a purged SourceSurfaceVolatileBuffer. r=jrmuizel

Backout by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f6d72c338e Backed out changeset 722569a89186 due to bug 1400549 and getting the crash signatures we needed.

The crash reports confirm that surfaces that were purged volatile buffers are remaining in the cache. It also demostrates the code path that this happens on. I'll set to work on a proper fix now.

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/b3f6d72c338e

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1154f3641315a9d0692b84266a299f794470aa80

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f5756b1d12 Part 1. Ensure SurfaceCache::CollectSizeOfSurfaces removes purged volatile buffer-backed surfaces. r=tnikkel

Comment on attachment 8909283 [details] [diff] [review] Part 2. Ensure SourceSurfaceVolatileData does not forget its purged state., v1 [carries r=jrmuizel] In retrospect, this is almost identical to attachment 8908717 [details] [diff] [review], save I replaced the diagnostic assert with a graceful failure.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/58844e6f37b2 Part 2. Ensure SourceSurfaceVolatileData does not forget its purged state. r=jrmuizel

Backed out for frequently asserting at image/SurfaceCache.cpp:138: https://hg.mozilla.org/integration/mozilla-inbound/rev/b577f883dc898d203b74f7b02345a0a7dc3d32c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0cd5f70746c4efeb84bb76ce32d19043145341 Push with first failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5fb440d47ab9ebfa9633ff88274f221924f3cc3a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable Failure log 1: https://treeherder.mozilla.org/logviewer.html#?job_id=131862744&repo=mozilla-inbound [task 2017-09-19T01:38:34.832Z] 01:38:34 INFO - TEST-START | dom/html/test/browser_bug592641.js [task 2017-09-19T01:38:34.990Z] 01:38:34 INFO - GECKO(2882) | ++DOCSHELL 0x7f55ecf31000 == 2 [pid = 3029] [id = {0b2b4bdd-c850-480f-9cf3-d5f7e5dbf310}] [task 2017-09-19T01:38:34.992Z] 01:38:34 INFO - GECKO(2882) | ++DOMWINDOW == 8 (0x7f55ecf33000) [pid = 3029] [serial = 21] [outer = (nil)] [task 2017-09-19T01:38:35.014Z] 01:38:35 INFO - GECKO(2882) | --DOCSHELL 0x7f64ec728000 == 1 [pid = 2928] [id = {f8db712a-c1e8-4411-9739-e03fa4ba8dae}] [task 2017-09-19T01:38:35.018Z] 01:38:35 INFO - GECKO(2882) | --DOMWINDOW == 7 (0x7f64ec085000) [pid = 2928] [serial = 15] [outer = (nil)] [url = about:blank] [task 2017-09-19T01:38:35.022Z] 01:38:35 INFO - GECKO(2882) | --DOMWINDOW == 6 (0x7f64ec740800) [pid = 2928] [serial = 18] [outer = (nil)] [url = about:blank] [task 2017-09-19T01:38:35.107Z] 01:38:35 INFO - GECKO(2882) | ++DOMWINDOW == 9 (0x7f55ee80b800) [pid = 3029] [serial = 22] [outer = 0x7f55ecf33000] [task 2017-09-19T01:38:36.317Z] 01:38:36 INFO - GECKO(2882) | [Parent 2882, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/netwerk/base/nsChannelClassifier.cpp, line 342 [task 2017-09-19T01:38:36.759Z] 01:38:36 INFO - GECKO(2882) | ++DOMWINDOW == 10 (0x7f55ef5c4000) [pid = 3029] [serial = 23] [outer = 0x7f55ecf33000] [task 2017-09-19T01:38:36.825Z] 01:38:36 INFO - GECKO(2882) | [Child 3029, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6389 [task 2017-09-19T01:38:36.836Z] 01:38:36 INFO - GECKO(2882) | [Child 3029, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6389 [task 2017-09-19T01:38:36.979Z] 01:38:36 INFO - GECKO(2882) | [Child 3029, Main Thread] WARNING: Subsequent listeners are not retargetable: file /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp, line 3351 [task 2017-09-19T01:38:37.040Z] 01:38:37 INFO - GECKO(2882) | --DOMWINDOW == 31 (0x7ffa5a9b7800) [pid = 2882] [serial = 17] [outer = (nil)] [url = about:blank] [task 2017-09-19T01:38:37.246Z] 01:38:37 INFO - GECKO(2882) | ++DOCSHELL 0x7f81dc022800 == 4 [pid = 2978] [id = {24353975-0618-4025-8b4d-baab82ee02af}] [task 2017-09-19T01:38:37.250Z] 01:38:37 INFO - GECKO(2882) | ++DOMWINDOW == 9 (0x7f81dcdec800) [pid = 2978] [serial = 17] [outer = (nil)] [task 2017-09-19T01:38:37.375Z] 01:38:37 INFO - GECKO(2882) | ++DOMWINDOW == 10 (0x7f81ddcc7800) [pid = 2978] [serial = 18] [outer = 0x7f81dcdec800] [task 2017-09-19T01:38:37.675Z] 01:38:37 INFO - GECKO(2882) | [Parent 2882, Main Thread] WARNING: NS_ENSURE_TRUE(aSecondURI) failed: file /builds/worker/workspace/build/src/dom/base/ThirdPartyUtil.cpp, line 98 [task 2017-09-19T01:38:37.845Z] 01:38:37 INFO - GECKO(2882) | [Parent 2882, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/netwerk/base/nsChannelClassifier.cpp, line 342 [task 2017-09-19T01:38:37.988Z] 01:38:37 INFO - GECKO(2882) | Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Called GetDrawableSurface() on a placeholder), at /builds/worker/workspace/build/src/image/SurfaceCache.cpp:138 [task 2017-09-19T01:38:37.990Z] 01:38:37 INFO - GECKO(2882) | #01: mozilla::image::ImageSurfaceCache::CollectSizeOfSurfaces<mozilla::image::SurfaceCacheImpl::CollectSizeOfSurfaces(mozilla::image::ImageKey, nsTArray<mozilla::image::SurfaceMemoryCounter>&, mozilla::MallocSizeOf, const StaticMutexAutoLock&)::<lambda(mozilla::NotNull<mozilla::image::CachedSurface*>)> > [image/SurfaceCache.cpp:631] [task 2017-09-19T01:38:37.991Z] 01:38:37 INFO - [task 2017-09-19T01:38:38.000Z] 01:38:37 INFO - GECKO(2882) | #02: mozilla::image::SurfaceCache::CollectSizeOfSurfaces [image/SurfaceCache.cpp:1189] [task 2017-09-19T01:38:38.003Z] 01:38:37 INFO - [task 2017-09-19T01:38:38.005Z] 01:38:37 INFO - GECKO(2882) | #03: mozilla::image::RasterImage::CollectSizeOfSurfaces [image/RasterImage.cpp:759] [task 2017-09-19T01:38:38.005Z] 01:38:37 INFO - [task 2017-09-19T01:38:38.006Z] 01:38:38 INFO - GECKO(2882) | #04: mozilla::image::ImageMemoryCounter::ImageMemoryCounter [xpcom/ds/nsTArray.h:398] [task 2017-09-19T01:38:38.006Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.016Z] 01:38:38 INFO - GECKO(2882) | #05: imgMemoryReporter::ImagesContentUsedUncompressedDistinguishedAmount [xpcom/ds/nsTArray.h:2234] [task 2017-09-19T01:38:38.021Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.024Z] 01:38:38 INFO - GECKO(2882) | #06: GetInfallibleAmount [xpcom/base/nsMemoryReporterManager.cpp:2439] [task 2017-09-19T01:38:38.035Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.040Z] 01:38:38 INFO - GECKO(2882) | #07: NS_InvokeByIndex [task 2017-09-19T01:38:38.043Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.046Z] 01:38:38 INFO - GECKO(2882) | #08: CallMethodHelper::Call [js/xpconnect/src/xpcprivate.h:775] [task 2017-09-19T01:38:38.054Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.056Z] 01:38:38 INFO - GECKO(2882) | #09: XPCWrappedNative::CallMethod [js/xpconnect/src/XPCWrappedNative.cpp:1282] [task 2017-09-19T01:38:38.061Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.064Z] 01:38:38 INFO - GECKO(2882) | #10: XPC_WN_GetterSetter [js/xpconnect/src/xpcprivate.h:1685] [task 2017-09-19T01:38:38.066Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.075Z] 01:38:38 INFO - GECKO(2882) | #11: js::CallJSNative [js/src/jscntxtinlines.h:294] [task 2017-09-19T01:38:38.078Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.084Z] 01:38:38 INFO - GECKO(2882) | #12: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:495] [task 2017-09-19T01:38:38.088Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.093Z] 01:38:38 INFO - GECKO(2882) | #13: js::Call [js/src/vm/Interpreter.cpp:559] [task 2017-09-19T01:38:38.095Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.100Z] 01:38:38 INFO - GECKO(2882) | #14: js::CallGetter [js/src/jsapi.h:74] [task 2017-09-19T01:38:38.102Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.107Z] 01:38:38 INFO - GECKO(2882) | #15: GetExistingProperty<(js::AllowGC)1u> [js/src/vm/NativeObject.cpp:2122] [task 2017-09-19T01:38:38.109Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.113Z] 01:38:38 INFO - GECKO(2882) | #16: NativeGetPropertyInline<(js::AllowGC)1u> [js/src/vm/NativeObject.cpp:2384] [task 2017-09-19T01:38:38.118Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.121Z] 01:38:38 INFO - GECKO(2882) | #17: js::GetProperty [js/src/vm/NativeObject.h:1541] [task 2017-09-19T01:38:38.123Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.126Z] 01:38:38 INFO - GECKO(2882) | #18: js::GetObjectElementOperation [js/src/vm/Interpreter-inl.h:524] [task 2017-09-19T01:38:38.131Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.134Z] 01:38:38 INFO - GECKO(2882) | #19: js::GetElementOperation [js/src/vm/Interpreter-inl.h:629] [task 2017-09-19T01:38:38.136Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.139Z] 01:38:38 INFO - GECKO(2882) | #20: Interpret [js/src/vm/Interpreter.cpp:2922] [task 2017-09-19T01:38:38.142Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.145Z] 01:38:38 INFO - GECKO(2882) | #21: js::RunScript [js/src/vm/Interpreter.cpp:406] [task 2017-09-19T01:38:38.150Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.154Z] 01:38:38 INFO - GECKO(2882) | #22: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:513] [task 2017-09-19T01:38:38.159Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.167Z] 01:38:38 INFO - GECKO(2882) | #23: Interpret [js/src/vm/Interpreter.cpp:3084] [task 2017-09-19T01:38:38.170Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.172Z] 01:38:38 INFO - GECKO(2882) | #24: js::RunScript [js/src/vm/Interpreter.cpp:406] [task 2017-09-19T01:38:38.174Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.180Z] 01:38:38 INFO - GECKO(2882) | #25: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:513] [task 2017-09-19T01:38:38.182Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.185Z] 01:38:38 INFO - GECKO(2882) | #26: Interpret [js/src/vm/Interpreter.cpp:3084] [task 2017-09-19T01:38:38.192Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.198Z] 01:38:38 INFO - GECKO(2882) | #27: js::RunScript [js/src/vm/Interpreter.cpp:406] [task 2017-09-19T01:38:38.201Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.203Z] 01:38:38 INFO - GECKO(2882) | #28: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:513] [task 2017-09-19T01:38:38.205Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.208Z] 01:38:38 INFO - GECKO(2882) | #29: js::Call [js/src/vm/Interpreter.cpp:559] [task 2017-09-19T01:38:38.210Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.214Z] 01:38:38 INFO - GECKO(2882) | #30: js::jit::InterpretResume [js/src/jsapi.h:74] [task 2017-09-19T01:38:38.220Z] 01:38:38 INFO - [task 2017-09-19T01:38:38.222Z] 01:38:38 INFO - GECKO(2882) | #31: ??? (???:???) Failure log 2: https://treeherder.mozilla.org/logviewer.html#?job_id=131917420&repo=mozilla-inbound

I missed a check to IsPlaceholder. Will add.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a23a810d9585 Part 1. Ensure SurfaceCache::CollectSizeOfSurfaces removes purged volatile buffer-backed surfaces. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff9cd026e70 Part 2. Ensure SourceSurfaceVolatileData does not forget its purged state. r=jrmuizel

https://hg.mozilla.org/mozilla-central/rev/a23a810d9585 https://hg.mozilla.org/mozilla-central/rev/3ff9cd026e70

Updated to a fresh nightly, and now I'm hitting your assert. See bp-5361c914-37c9-4f32-890a-c82990170920

(In reply to Adam Roach [:abr] from comment #35) > Updated to a fresh nightly, and now I'm hitting your assert. See > bp-5361c914-37c9-4f32-890a-c82990170920 Never mind -- my previous update apparently didn't take me all the way to today's build -- this was based on a 09-16 build. I've updated, verified today's date in "About Nightly", and will keep my eyes open for related issues.