1093708 - Crash with missing scrollbar image in resource constrained builds

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Nick and I have seen this. Looks like there's a resource not making it into the build when it should.

Comment 1

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 2

[Nick Alexander :nalexander [he/him]]

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.

Comment 3

[Richard Newman [:rnewman]]

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.

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/92713f2cea3a

Comment 5

[:Margaret Leibovic]

Backed out for breaking the build:
https://hg.mozilla.org/integration/fx-team/rev/25ad77f01193

https://tbpl.mozilla.org/php/getParsedLog.php?id=51816812&tree=Fx-Team

Comment 6

[Richard Newman [:rnewman]]

Attachment: Include mdpi assets in constrained builds. v2

This ought to work. Testing.

Comment 7

[Nick Alexander :nalexander [he/him]]

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.

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/91cabcd5d701

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91cabcd5d701

Comment 10

[Richard Newman [:rnewman]]

Nick: do you think this is relevant?

   --auto-add-overlay
       Automatically add resources that are only in overlays.

Comment 11

[Nick Alexander :nalexander [he/him]]

(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.