Crash with missing scrollbar image in resource constrained builds

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Nick and I have seen this. Looks like there's a resource not making it into the build when it should.
(Assignee)

Comment 1

3 years ago
Created attachment 8516936 [details] [diff] [review]
Include mdpi assets in constrained builds. v1

I feel like this shouldn't be necessary (why can't aapt understand Android resource resolution and decide that there's no hdpi resource shadowing the scrollbar image, and decide to include it?), and yet with this patch we get a scrollbar drawable in the output APK.
Attachment #8516936 - Flags: review?(nalexander)
Comment on attachment 8516936 [details] [diff] [review]
Include mdpi assets in constrained builds. v1

Review of attachment 8516936 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this -- you basically own this configuration right now -- but are you certain this is the correct approach?

I think we need to guarantee that the total set of resources is present regardless of the -c flags.  What's weird is that this build is using an R.java produced with the -c flags; it's not a runtime mismatch between R.java and the packed resources because R.java is (should be) reflecting the packed resources.

This feels like an aapt bug: it should recognize a missing resource and complain.  Either that, or we should find a way to make sure the actual APK has the resources that R.java says it has.

For right now, how do we know that the union of hdpi and mdpi is sufficient?  In any case, land to unbork, but let's think of follow-up to make this better.
Attachment #8516936 - Flags: review?(nalexander) → review+
(Assignee)

Comment 3

3 years ago
I totally think that this is *not* the correct approach!

Indeed, this is something that I think aapt should be doing, and yes, we will probably hit this again.

In the short to medium term, we'll at least have a build go red on treeherder when someone gets this wrong.
(Assignee)

Comment 6

3 years ago
Created attachment 8516988 [details] [diff] [review]
Include mdpi assets in constrained builds. v2

This ought to work. Testing.
Attachment #8516988 - Flags: review?(nalexander)
Comment on attachment 8516988 [details] [diff] [review]
Include mdpi assets in constrained builds. v2

Review of attachment 8516988 [details] [diff] [review]:
-----------------------------------------------------------------

Building without constrained resources was fine for me.
Attachment #8516988 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/91cabcd5d701
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 10

3 years ago
Nick: do you think this is relevant?

   --auto-add-overlay
       Automatically add resources that are only in overlays.
(In reply to Richard Newman [:rnewman] from comment #10)
> Nick: do you think this is relevant?
> 
>    --auto-add-overlay
>        Automatically add resources that are only in overlays.

Probably not.  Suppose you have two res directories, A and B, and B included something that isn't in A.  With --auto, the extra resource in B is included; without, the extra resource is not included (since it doesn't *overlay* a thing in A).  Since we want the union of all the given sets of resources, we need this.

My guess is we'd see problems earlier (and hopefully at build time) if we didn't dynamically load this resource.
You need to log in before you can comment on or make changes to this bug.