Closed Bug 694237 Opened 13 years ago Closed 13 years ago

Reenable downloadable fonts for 10.7.2 users

Categories

(Camino Graveyard :: General, defect)

1.9.2 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino2.1

People

(Reporter: phiw2, Assigned: alqahira)

Details

Attachments

(1 file, 1 obsolete file)

Bug 663688 disabled downloadable fonts on 10.7 due to bugs in ATS. (see http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0a1993a0c859).

With the release of 10.7.2, the underlying Apple bug appears fixed (according the rel. notes for sec. update 2011-006 – http://support.apple.com/kb/HT5002)

From the rel notes:
> Available for: Mac OS X v10.6.8, Mac OS X Server v10.6.8, OS X Lion v10.7 and v10.7.1, OS X Lion Server v10.7 and v10.7.1
> Impact: Applications which use the ATSFontDeactivate API may be vulnerable to an unexpected application termination or arbitrary code execution
> Description: A buffer overflow issue existed in the ATSFontDeactivate API.
> CVE-ID
> CVE-2011-0230 : Steven Michaud of Mozilla


[11:27am] sauron: awesome
[11:28am] sauron: so, worst-case, if it is fixed in 10.7.2, we can flip the pref ourselves
[11:29am] sauron: either in all-camino.js, or in code to make sure it's only done on 10.7.2 and up
[11:29am] sauron: (gfx_downloadable_fonts.enabled.lion)

(I'm keeping an eye on core if they flip the pref themselves)
Flags: camino2.1?
Given the severity of the issue and the relative ease of hitting the crash, I think that if we have to do this ourselves (i.e., if Core doesn't flip the pref), we should conditionalize it only on 10.7.x ≥ 2, rather than setting it unconditionally and plaguing our users who will upgrade Camino but not the OS (grr) with serious crashes.

The first step here is to find someone on 10.7.2 who will flip the gfx_downloadable_fonts.enabled.lion pref to true and then browse tons of pages with downloadable fonts, in order to verify that the crash is fixed in our context.  I'm not sure if it's safe to disclose Steven's pageset cycler URL for bug 663688 publicly, but someone on 10.7.2 who wants to test this can ping me and I'll pass it on.
We absolutely need to check for 10.7.2 (assuming it's actually fixed; has someone tested yet?) given that we're going to have these fonts on our own website. I really think we should get this into 2.1 final.
(In reply to Samuel Sidler (:ss) from comment #2)
> We absolutely need to check for 10.7.2 (assuming it's actually fixed; has
> someone tested yet?)

We have an extreme lack of people on 10.7 willing to perform a simple test, apparently. :(  The forum post has gotten lots of reads but no replies in the past week.

> I really think we should get this into 2.1 final.

To do this, we need to get testing ASAP, because I'd like the fix (which should be pretty simple to write) to bake for a few days before we build the RC.
I'll test. (I don't regularly read the forum.) What's needed?
(In reply to Samuel Sidler (:ss) from comment #4)
> I'll test. (I don't regularly read the forum.) What's needed?

1) 10.7.2
2) Camino 2.1b2 or newer
3) Flip "gfx_downloadable_fonts.enabled.lion" to true
4) Load the fonts page cycler from bug 663688 and let it cycle for a while
5) Don't crash???

Check your email ;)
Additional tests:

* Load http://www.google.com/webfonts and play around a little
* visit sites listed http://blog.typekit.com/category/sites-we-like/ in multiple tabs, close tabs, quit / relaunch Camino with multiple tabs open

(at least we know the typekit fonts are good quality…)

don't crash :-)
Here is a pageset that should work better:
http://dev.l-c-n.com/camino/b694237/index.html
And if the timeout is still not long enough for most pages to load entirely for you (philippe bumped it up for me when I tested, so that most pages loaded fully the first time they were visited, or at worst on the second time), you can use Firebug Lite to redefine pageTimeout to a larger value, or save the loader stuff locally and edit it by hand.
Most pages loaded fully for me the second time they were loaded. I let the script run for a good 30 minutes with no crashes. I'm going to let it run tonight though, for at least three hours and see how it does. I think with a longer timeout it needs to run more than 30 minutes to ensure there won't be any crashes.
I "accidentally" left the script running for five hours... but it didn't crash!
(In reply to Samuel Sidler (:ss) from comment #10)
> I "accidentally" left the script running for five hours... but it didn't
> crash!

That makes me feel much better; thanks for testing!  I'd still like to get the patch into testing for a few days before we spin RC.

I whipped up this patch; I think it does the right thing in the common case.  Some questions, though:

> +  if (mLastRunPrefsVersion < 6 && [NSWorkspace isLionR2OrHigher]) {

This is going to do the right thing on new profiles, right?  It will enable downloadable fonts for 10.7 users making a new profile on 10.7≥2, because in a profile-less situation |mLastRunPrefsVersion| is 0 when we don't fetch a |lastRunPrefsVersion|, right?

This won't do the right thing if someone is still on 10.7/10.7.1 and later finally upgrades to 10.7.2 or higher, which is not unheard-of, especially since 10.7.2 just came out last week; we could handle this case by dropping the prefs version check entirely and just running on every launch, I guess.  Do we want to handle that case?

> -// other Mozilla/Firefox values are not implement in Camino
> +// other Mozilla/Firefox values are not implemented in Camino

I'll land this typo fix separately; I just wanted to catch it once I had noticed it (and be able to get sr+ on it :P ).
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Attachment #568497 - Flags: superreview?(stuart.morgan+bugzilla)
Comment on attachment 568497 [details] [diff] [review]
Re-enable web fonts for 10.7.2 users

>++ (BOOL)isLionR2OrHigher;

This file should be for general purpose checks; "LionR2" is something we'll never use again, so just inline the check in prefs.

>+  if (mLastRunPrefsVersion < 6 && [NSWorkspace isLionR2OrHigher]) {
>+    // Core disabled downloadable fonts on 10.7 due to an OS bug that Apple
>+    // fixed in 10.7.2.  Re-enable downloadable fonts for Lion users running
>+    // 10.7.2 or higher.
>+    [self setPref:kGeckoPrefEnableWebFontsOnLion toBoolean:YES];

This won't do the right thing for anyone running anything less than 10.7.2 (say, 10.6) when they install Camino 2.1, who later upgrades to Lion, which doesn't seem ideal.

I think what we want instead is to forcibly unfork the prefs by setting kGeckoPrefEnableWebFontsOnLion to the value of kGeckoPrefEnableWebFonts on *every* launch on 10.7.2+. That way, for users with the fix the original pref will Just Work (modulo needing to flip both in about:config, but that's enough of an edge case, and easy enough to do, that I don't really care), and the ordering of Camino vs OS updates won't matter.
Attachment #568497 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Oops, I missed that you discussed that when posting the patch. But it's much worse than just 10.7.0/1, since it also happens for 10.[4-6].x

> we could handle this case by dropping the prefs version check entirely and just running on every launch, I guess.

But we don't want to make web fonts impossible to turn off on Lion, which is why I think we should use the original pref as the value.
(In reply to Stuart Morgan from comment #13)
> Oops, I missed that you discussed that when posting the patch. But it's much
> worse than just 10.7.0/1, since it also happens for 10.[4-6].x

Yeah, I totally forgot about those users :giant-oops:

> > we could handle this case by dropping the prefs version check entirely and > > just running on every launch, I guess.
> 
> But we don't want to make web fonts impossible to turn off on Lion, which is
> why I think we should use the original pref as the value.

I did think about that limitation of dropping the prefs version check but forgot to mention it when I discussed that option.  I agree that what we want to do is what you proposed, though, for exactly that reason.

The only wrinkle to doing exactly as you said in comment 12 is that NSWorkspace's |+systemVersion| wasn't public, so I made it public in order to inline the version comparison and not have to duplicate the gestalt code.  I hope that's OK.

I'm not always a fan of the ternary, but I liked the way it simplified things in the download pref migration code, so I copied that style here.

> -// other Mozilla/Firefox values are not implement in Camino
> +// other Mozilla/Firefox values are not implemented in Camino

Again, I'll land this typo fix separately (repeating this comment as a reminder to self).
Attachment #568497 - Attachment is obsolete: true
Attachment #568594 - Flags: superreview?(stuart.morgan+bugzilla)
Comment on attachment 568594 [details] [diff] [review]
Re-enable web fonts for 10.7.2 users, no matter when they upgrade

sr=smorgan
Attachment #568594 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
http://hg.mozilla.org/camino/rev/7a22333bedcb for the code, http://hg.mozilla.org/camino/rev/72636bf705de for the typo fix.

Thanks, everyone!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: camino2.1? → camino2.1+
Target Milestone: --- → Camino2.1
Core's finally filed their bug to investigate this: bug 696702

If they stick to their current plan of making the ".lion" pref only apply to 10.7 and 10.7.1, then we can back out my patch (although it won't break things if it stays in; we'll just end doing unnecessary work).

As it stands now, though, there's nothing in the 2.1b3pre crashes since this landed[1] that seems like it could be that crash; I'd like to give it a day or two more and then RC, rather than wait for Core to decide to actually test things and debate writing a patch and landing it. 

[1] https://crash-stats.mozilla.com/query/query?product=Camino&version=Camino%3A2.1b3pre&platform=mac&range_value=3&range_unit=days&date=now&query_search=signature&query_type=exact&query=&reason=&build_id=&process_type=any&hang_type=any&do_query=1# (but note also that that's all 2.1b3pre builds, not just the 3 with this fix in them, because you can't do a range of builds, just a given build.)
(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #17)
> I'd like to give it a day or two more and then RC, rather than wait for Core
> to decide to actually test things and debate writing a patch and landing it. 

+1
Core's bug 696702 finally landed about a week ago and will be in 1.9.2.5; we'll want to back this out going forward.
(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #19)
> Core's bug 696702 finally landed about a week ago and will be in 1.9.2.5;
> we'll want to back this out going forward.

yeah, I filed bug 704422 for that.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: