Closed
Bug 1111041
Opened 9 years ago
Closed 9 years ago
no raster images displayed in trunk builds (browser console says "image corrupt or truncated")
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: mpk, Assigned: seth)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
2.15 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No raster images (gif, jpeg, png, ico) are displayed in recent trunk builds, at least not on FreeBSD 10 stable (amd64) with up-to-date ports. Vector graphics (svg) still work fine. But even the favicons and all the toolbar and navigation images are missing. Using a new profile doesn't help at all. I was able to narrow down the regression window: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e0783737421&tochange=d51509bc5cfd Could this be the result of the changes introduced with either bug 1060869 or bug 1057904?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(seth)
Keywords: regression
Reporter | ||
Comment 1•9 years ago
|
||
Looks like the regression window is even smaller: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e0783737421&tochange=7ee7e774e19f
Assignee | ||
Comment 2•9 years ago
|
||
I'm guessing bug 1060869 is the culprit. Unfortunately, I'm not sure how this could happen - these patches have been tested on many platforms and I haven't heard of this problem anywhere else. For now it seems reasonable to assume that there's something FreeBSD-specific about this issue. One way things could go wrong is if you are somehow getting the wrong prefs set. Could you post the value you have for the "image.mem.surfacecache.max_size_kb", "image.mem.surfacecache.size_factor", "image.mem.surfacecache.discard_factor", and "image.mem.surfacecache.min_expiration_ms" prefs? Another possible issue could be that something is going wrong with the surface cache size calculation code. The total amount of memory installed on your machine is a factor in that calculation, and that value is retrieved using platform-specific APIs. It's possible that's busted on FreeBSD. If you're comfortable with coding, could you either use a debugger or printf logging to get the values of "proposedSize", "surfaceCacheSizeBytes", and "finalSurfaceCacheSizeBytes" in the function "SurfaceCache::Initialize" in image/src/SurfaceCache.cpp? Maybe also determine what "PR_GetPhysicalMemorySize" returns? Having those values would make it clear right away if there's a problem with the size calculation code.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail)
Reporter | ||
Comment 3•9 years ago
|
||
Thanks for the pointers, Seth. Yes, the physical memory size doesn't get calculated , e.g. PR_GetPhysicalMemorySize() always returns 0. I did have to modify nsprpub/pr/src/misc/prsystem.c in order to get everything going again. Patch is coming up soon...
Assignee: nobody → bugmail
Blocks: 1060869
Status: NEW → ASSIGNED
Flags: needinfo?(seth)
Flags: needinfo?(bugmail)
Reporter | ||
Comment 4•9 years ago
|
||
This patch extends the function PR_GetPhysicalMemorySize() in the file nsprpub/pr/src/misc/prsystem.c with code for FreeBSD. The only difference between FreeBSD and Net/OpenBSD seems to be that we have to use HW_PHYSMEM while they seem to use HW_PHYSMEM64. We should probably also enable the same function for DragonflyBSD, but I'm not sure what is defined in that case. HW_PHYSMEM may even be available on all BSDs and support 64bit values. If so, we could extend _PR_HAVE_SYSCTL and check for that.
Attachment #8536030 -
Flags: review?(wtc)
Comment on attachment 8536030 [details] [diff] [review] implement PR_GetPhysicalMemorySize() for FreeBSD Review of attachment 8536030 [details] [diff] [review]: ----------------------------------------------------------------- Same as bug 782124 ? ::: nsprpub/pr/src/misc/prsystem.c @@ +292,5 @@ > +#elif defined(FREEBSD) > + > + int mib[2]; > + int rc; > + uint64_t memSize; unsigned long != uint64_t on 32bit archs. HW_PHYSMEM returns |unsigned long| on DragonFly and FreeBSD. HW_REALMEM returns |unsigned long| on FreeBSD as well. HW_PHYSMEM returns |unsigned int| on NetBSD and OpenBSD while HW_PHYSMEM64 returns |uint64_t|. The latter is similar to HW_MEMSIZE on OS X.
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8536030 [details] [diff] [review] implement PR_GetPhysicalMemorySize() for FreeBSD Yes, indeed. But your patch looks way better across the field. No need to have two reviews open for the same thing. :-) The lack of PR_GetPhysicalMemorySize() has really started to bite now. I wonder whether we should assume a minimum of at least 256MB in SurfaceCache.
Attachment #8536030 -
Attachment is obsolete: true
Attachment #8536030 -
Flags: review?(wtc)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Marco Perez [:another_loco] from comment #6) > The lack of PR_GetPhysicalMemorySize() has really started to bite now. > I wonder whether we should assume a minimum of at least 256MB in > SurfaceCache. Well, other parts of Gecko use PR_GetPhysicalMemorySize(), so the best solution is to fix it. That said I am absolutely willing to add code to detect the case where PR_GetPhysicalMemorySize() returns 0 and use a fallback size. I would be inclined to make that code path fatally assert in debug builds, though.
Assignee | ||
Comment 8•9 years ago
|
||
Per comment 7, this patch implements a fallback for PR_GetPhysicalMemorySize which fatally asserts in debug mode. I want to make sure that we detect and fix this problem on platforms where it occurs. (Especially since the fallback will provide a suboptimal browsing experience, since the cache size is artifically limited!)
Attachment #8536788 -
Flags: review?(dholbert)
Assignee | ||
Updated•9 years ago
|
Assignee: bugmail → seth
Reporter | ||
Comment 9•9 years ago
|
||
Thanks for the great work, Seth, I'm sure some contributors for the tier-2 and especially the tier-3 platforms will greatly appreciate it. It'll sure make porting/debugging quite a bit easier. :-)
Assignee | ||
Comment 10•9 years ago
|
||
Glad to help! =)
Comment 11•9 years ago
|
||
Comment on attachment 8536788 [details] [diff] [review] Detect PR_GetPhysicalMemorySize failure in SurfaceCache >diff --git a/image/src/SurfaceCache.cpp b/image/src/SurfaceCache.cpp >--- a/image/src/SurfaceCache.cpp >+++ b/image/src/SurfaceCache.cpp >- uint64_t proposedSize = PR_GetPhysicalMemorySize() / surfaceCacheSizeFactor; >+ uint64_t memorySize = PR_GetPhysicalMemorySize(); >+ if (memorySize == 0) { >+ NS_NOTREACHED("PR_GetPhysicalMemorySize not implemented on this platform"); >+ memorySize = 256 * 1024 * 1024; // Fall back to 256MB. >+ } NS_NOTREACHED isn't fatal -- it's a version of NS_ASSERTION. Use MOZ_ASSERT_UNREACHABLE, if you want this to be fatal (in debug builds). r=me with that.
Attachment #8536788 -
Flags: review?(dholbert) → review+
Comment 12•9 years ago
|
||
And while you're at it, SurfaceCache.cpp needs the following #include to use MOZ_ASSERT / MOZ_ASSUME_UNREACHABLE: #include "mozilla/Assertions.h" It looks like we're already indirectly getting that #include via some other header (since we have MOZ_ASSERT in the file already, and that's not breaking the build.) But we should remove that indirect dependency & make the #include explicit.
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #11) > NS_NOTREACHED isn't fatal -- it's a version of NS_ASSERTION. Use > MOZ_ASSERT_UNREACHABLE, if you want this to be fatal (in debug builds). Hmm, I have probably misused this macro elsewhere. At any rate, I'll make those changes. Thanks for the review!
Assignee | ||
Comment 14•9 years ago
|
||
This version addresses Daniel's review comments.
Assignee | ||
Updated•9 years ago
|
Attachment #8536788 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/13b2284f09c9
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/13b2284f09c9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8538972 [details] [diff] [review] Detect PR_GetPhysicalMemorySize failure in SurfaceCache Approval Request Comment [Feature/regressing bug #]: 1060869 [User impact if declined]: People running Firefox to non-Tier 1 platforms may find that no raster images are displayed, with no obvious error reported to help them fix the problem. [Describe test coverage new/current, TBPL]: On m-c. [Risks and why]: Very low risk. [String/UUID change made/needed]: None.
Attachment #8538972 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8538972 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•