Closed Bug 1212889 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox41 unaffected, firefox42 unaffected, firefox43 affected, firefox44 affected, firefox45 fixed, fennec43+)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox41 --- unaffected
firefox42 --- unaffected
firefox43 --- affected
firefox44 --- affected
firefox45 --- fixed
fennec 43+ ---

People

(Reporter: u421692, Assigned: Margaret)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
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
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?
Blocks: 1210611
tracking-fennec: --- → ?
Flags: needinfo?(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?
Flags: needinfo?(dolske) → needinfo?(margaret.leibovic)
Assignee: nobody → dolske
tracking-fennec: ? → 43+
(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.
Oh even more suspiciously pointing to bug 1210611, as it seems that landed in both central and aurora.
Of course this seems to identify a deficiency in that there does not appear to be any test that ensures that about:rights works.
(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(mark.finkle)
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.
Assignee: dolske → margaret.leibovic
Blocks: 1198525
No longer blocks: 1210611
Bug 1212889 - Always use chrome://browser/content/aboutRights.xhtml to ensure mobile version of about:rights. r=mfinkle
Attachment #8688141 - Flags: review?(mark.finkle)
I found this piece of history:
https://bugzilla.mozilla.org/show_bug.cgi?id=609069#c2
Attachment #8688141 - Flags: review?(mark.finkle) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/054a1e6aa987
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(mark.finkle)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.