Closed Bug 1111041 Opened 5 years ago Closed 5 years ago

no raster images displayed in trunk builds (browser console says "image corrupt or truncated")

Categories

(Core :: ImageLib, defect)

All
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: mpk, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(seth)
Keywords: regression
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.
Flags: needinfo?(bugmail)
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)
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.
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)
Depends on: 782124
Hardware: x86_64 → All
(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.
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: bugmail → seth
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. :-)
Glad to help! =)
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+
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.
(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!
This version addresses Daniel's review comments.
Attachment #8536788 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/13b2284f09c9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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?
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.