1212889 - "undefined entity' when trying to open "about:rights" page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

1. Open "about:rights" page

Actual result:
The following message is displayed:

XML Parsing Error: undefined entity
Location: jar:jar:file:///data/app/org.mozilla.fennec-2/base.apk!/assets/omni.ja!/chrome/toolkit/content/global/aboutRights.xhtml" 
Line Number 20, Column 10:
<title>&rights.pagetitle;</title>

Logcat:
E/GeckoConsole(899): [JavaScript Error: "undefined entity" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2/base.apk!/assets/omni.ja!/chrome/toolkit/content/global/aboutRights.xhtml" line: 20 column: 10 source: "  <title>&rights.pagetitle;</title>"}]

Good build: 2015-10-03
Bad build: 2015-10-04
Pushlog:http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0010c0cb259e28faf764949df54687e3a21a2d0a&tochange=8464f02a2798cf9ff8759df712f2c77ec0b15d23

Comment 1

[:Margaret Leibovic]

I suspect this has something to do with how we package strings, since the HTML page is located in /mobile, but the strings are in /toolkit:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutRights.xhtml
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/aboutRights.dtd

Or I wonder if this might have been caused by bug 1210611... I notice that toolkit has an aboutRights-unbranded.xhtml, but we don't have a mobile override for that:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/jar.mn#22

Comment 2

[:Margaret Leibovic]

I can't reproduce this in a local build... which makes me extra suspicious of bug 1210611. And actually reading the summary there makes it clear that it's about about:rights!

dolske, any ideas?

Comment 3

[Justin Dolske [:Dolske]]

Hmm.

Looking at the mozilla-release MXR (41, so before I starting futzing with things!), I see

http://mxr.mozilla.org/mozilla-release/source/mobile/android/components/AboutRedirector.js#36

36   rights: {
37     uri: AppConstants.MOZ_OFFICIAL_BRANDING ?
38       "chrome://browser/content/aboutRights.xhtml" :
39       "chrome://global/content/aboutRights-unbranded.xhtml",
40     privileged: false
41   },

In bug 1198525 I removed the "-unbranded" here. That bug made the toolkit "aboutRights.xhtml" packaging name canonical, as it pulls in the correct content at build time, instead of packaging both and deciding at runtime. Except that didn't quite work as expected, because the #ifdef MOZILLA_OFFICIAL was never being hit (ie, the toolkit aboutRights.xhtml was _always_ the unbranded flavor), leading to the fix in bug 1210611.

But both flavors of the toolkit page use the &rights.pagetitle; entity, so I'm confused why this would be broken now. Did it ever work?

Also, I just noticed that the Android AboutRedirector.js is using MOZ_OFFICIAL_BRANDING, which is different than MOZILLA_OFFICIAL. This should be changed, but shouldn't have any impact on this bug. The (subtle?) difference is that MOZ_OFFICIAL_BRANDING is used to control if the app has Firefox(tm) branding or not, and MOZILLA_OFFICIAL essentially indicates if it's a official build by Mozilla.

It might be helpful to know, in the broken build, what MOZ_OFFICIAL_BRANDING and MOZILLA_OFFICIAL are set to, and what the actual contents of chrome://global/content/aboutRights.xhtml is ("unbranded" or not).

Also, what's going on with the Android jar.mn overrides of aboutRights.dtd? Seems like it's just trying to provide an alternate chrome:// for the same toolkit .dtd? (I certainly don't see a mobile-specific aboutRights.dtd in the tree.) Does chrome://global/locale/aboutRights.dtd work at all on Android?

Comment 4

[Bill Gianopoulos [:WG9s]]

(In reply to :Margaret Leibovic from comment #2)
> I can't reproduce this in a local build... which makes me extra suspicious
> of bug 1210611. And actually reading the summary there makes it clear that
> it's about about:rights!
> 
> dolske, any ideas?

Exactly in  a local build it works fine.  This issue has existed for a while as it also exists in current Aurora builds, which is where I first noticed it.

Comment 5

[Bill Gianopoulos [:WG9s]]

Oh even more suspiciously pointing to bug 1210611, as it seems that landed in both central and aurora.

Comment 6

[Bill Gianopoulos [:WG9s]]

Of course this seems to identify a deficiency in that there does not appear to be any test that ensures that about:rights works.

Comment 7

[:Margaret Leibovic]

(In reply to Justin Dolske [:Dolske] from comment #3)
> Hmm.
> 
> Looking at the mozilla-release MXR (41, so before I starting futzing with
> things!), I see
> 
> http://mxr.mozilla.org/mozilla-release/source/mobile/android/components/
> AboutRedirector.js#36
> 
> 36   rights: {
> 37     uri: AppConstants.MOZ_OFFICIAL_BRANDING ?
> 38       "chrome://browser/content/aboutRights.xhtml" :
> 39       "chrome://global/content/aboutRights-unbranded.xhtml",
> 40     privileged: false
> 41   },
> 
> In bug 1198525 I removed the "-unbranded" here. That bug made the toolkit
> "aboutRights.xhtml" packaging name canonical, as it pulls in the correct
> content at build time, instead of packaging both and deciding at runtime.
> Except that didn't quite work as expected, because the #ifdef
> MOZILLA_OFFICIAL was never being hit (ie, the toolkit aboutRights.xhtml was
> _always_ the unbranded flavor), leading to the fix in bug 1210611.
> 
> But both flavors of the toolkit page use the &rights.pagetitle; entity, so
> I'm confused why this would be broken now. Did it ever work?

It must somehow work, because this works in beta/release, as well as in my local fx-team build.

> Also, I just noticed that the Android AboutRedirector.js is using
> MOZ_OFFICIAL_BRANDING, which is different than MOZILLA_OFFICIAL. This should
> be changed, but shouldn't have any impact on this bug. The (subtle?)
> difference is that MOZ_OFFICIAL_BRANDING is used to control if the app has
> Firefox(tm) branding or not, and MOZILLA_OFFICIAL essentially indicates if
> it's a official build by Mozilla.
> 
> It might be helpful to know, in the broken build, what MOZ_OFFICIAL_BRANDING
> and MOZILLA_OFFICIAL are set to, and what the actual contents of
> chrome://global/content/aboutRights.xhtml is ("unbranded" or not).

It's broken in Nightly, where MOZILLA_OFFICIAL is 1:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/config/mozconfigs/android-api-11/nightly

But this isn't built with official branding, so maybe that's part of the problem?

> Also, what's going on with the Android jar.mn overrides of aboutRights.dtd?
> Seems like it's just trying to provide an alternate chrome:// for the same
> toolkit .dtd? (I certainly don't see a mobile-specific aboutRights.dtd in
> the tree.) Does chrome://global/locale/aboutRights.dtd work at all on
> Android?

Yes, I can view-source chrome://global/locale/aboutRights.dtd on both Nightly and my local build.

I believe we selectively include strings from toolkit to avoid packaging unnecessary strings, but I'm not sure about the history of these overrides. Maybe mfinkle can provide more context.

Comment 8

[:Margaret Leibovic]

Aha! Looking more closely at view source, I see that the toolkit version of aboutRights.xhtml is what's being loaded in Nightly (busted), but the mobile version is what's being loaded in my local build (working).

This is wrong:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#37

We should be using chrome://browser/content/aboutRights.xhtml in either case.

Why did we even leave in that if statement with two different versions (browser/global)? Was this just an oversight?

This must be a regression from bug 1198525.

Comment 9

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1212889 - Always use chrome://browser/content/aboutRights.xhtml to ensure mobile version of about:rights. r=mfinkle

Bug 1212889 - Always use chrome://browser/content/aboutRights.xhtml to ensure mobile version of about:rights. r=mfinkle

Comment 10

[:Margaret Leibovic]

I found this piece of history:
https://bugzilla.mozilla.org/show_bug.cgi?id=609069#c2

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8688141 [details]
MozReview Request: Bug 1212889 - Always use chrome://browser/content/aboutRights.xhtml to ensure mobile version of about:rights. r=mfinkle

https://reviewboard.mozilla.org/r/25295/#review22821

This change seems to remove any hint of an unbranded rights page in Fennec, which is probably the reality anyway.

If an unbranded rights page is really required, we'll deal with it later.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/054a1e6aa987a67ae633f96db2f7aaf181a92dcd
Bug 1212889 - Always use chrome://browser/content/aboutRights.xhtml to ensure mobile version of about:rights. r=mfinkle

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/054a1e6aa987