667441 - Blocklisting isn't working due to mismatch between order of component usage and component registration

Status: RESOLVED FIXED

(Camino Graveyard :: Plug-ins, defect)

Description

[Stuart Morgan]

Now that we'll be doing security-based plugin blocking (bug 662666), we really need a more reliable way of forcing plugin state updates when Camino is updated. I'll look again to see if I can find a way of poking the plugin system; failing that maybe we'll delete pluginreg.dat on the first quit after each update, so the second launch will rebuild (although I'd like to avoid it if we did an autoupdate and deleted it *before* the update already)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

One other thought I had: camino.disabled_plugin_names works "live"; can we use the same mechanism that pref uses to make Core learn about state changes to learn about state changes on launch due to blocklist updates?

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(Somehow this was the only camino2.1+ bug that wasn't also Camino 2.1 milestone.)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I tried to dig around with this, again with little luck.  It's easy to find where Firefox pushes state updates to pluginHost (in nsBlocklistService.js, via the private copy of |GetPluginBlocklistState|, but the magic incantation that makes the new states take effect is still elusive.

To see if it involved a restart, I tried using various sample blocklists per the assorted fixed bugs (using extensions.blocklist.url, in Fx 3.6 and 3.0, and with a 1s update-check time), to no avail.  The only way I was able to blocklist a plug-in was to edit the on-disk blocklist.xml, at which point Firefox noticed on launch (so, no restart required).

Er, wow; I finally had a network check work; that 1s check interval took about 10 minutes :P  The alert just popped up in front of my Camino window :P  So Fx alerted me QuickTime was blocked (and suggested I restart to completely disable the plug-in).  I declined, and opening the Extensions Manager showed it blocked; visiting Apple's trailers page gave me Apple's "download QT" fallback content (so it's at least showing up as uninstalled to Apple's QT detection routines), but http://www.computerhope.com/issues/ch000591.htm#4 showed me the "blocked for your protection" Gecko placeholder.

So, there's got to be some way of poking the plug-in system into checking for updated blocked plug-ins (just like camino.disabled_plugin_names disables things live), we're just overlooking it somehow.

Some bugs I looked at: 
* bug 330511 (pluginHost support for blocklisted state)
* bug 391633 (blocklist webservice only)
* bug 271559 (hooked the blocklisting support in pluginHost into the blocklist service)
* bug 419582 (re-enabling an unblocklisted plug-in)
* bug 455906 (severities in blocklist)

I also noticed that pluginHost has some interesting methods, Get/SetBlocklisted (which appear never to be called) and UpdatePluginInfo.

Oh, from bug 455906, I wondered about this "state" value that's getting passed around: http://hg.mozilla.org/releases/mozilla-1.9.2/diff/8291b82dd323/toolkit/mozapps/extensions/src/nsBlocklistService.js#l1.489 http://hg.mozilla.org/releases/mozilla-1.9.2/diff/8291b82dd323/modules/plugin/base/src/nsPluginHostImpl.cpp#l1.61

Any chance that's our magic incantation?

Comment 4

[Stuart Morgan]

Get/SetBlocklisted should be the backing for setting the disabled property on an nsIPluginTag. We could probably manually re-run our own blocklist, but I don't think that really gets us where we really want to be since we'd still have the "stale plugin version" problem.

I still don't see a good way of forcing the plugin system to reconstruct everything if it doesn't think the plugins on disk have changed :(

Comment 5

[Stuart Morgan]

Although I guess that would change the bug from not blocking dangerous versions to blocking safe versions (if the plugin system has a stale version); maybe we should combine that with the post-first-launch file nuking so that someone annoyed by an overblock who tried quitting and relauching would get the fix.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I guess maybe comment 4/5 are suggesting what I was thinking tonight after tracing the camino.disabled_plugin_names path.

If we had PreferenceManager call a method |updatePluginBlocklistState| (similar to the existing |updatePluginEnableState|) at launch, we could then be able to force an update in PluginHost before Gecko web content encountered a plug-in and decided (not) to ask the blocklist service about the plug-in.  It'd suck to have to duplicate the code in the blocklist service impl (or, I guess, refactor it), but I think it would get us a better result.

And I'd rather us block safe versions accidentally/temporarily than leave dangerous versions unblocked…

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, when writing the patch for bug 727667, it appeared not to be working, and I couldn't figure out why (or what was wrong with my code).  Then I remembered this bug.

Since I was testing with a fresh profile, I should have had a completely fresh pluginreg.dat getting built and consulting our blocklist, but Flash wasn't being blocked.  I launched again with the same profile, and still no luck.  I deleted pluginreg.dat and restarted again with the same profile, still not blocked.  So I don't think anything related to manipulating pluginreg.dat is going to help, since apparently we still aren't being consulted until after Gecko has already decided the plug-in folder/plug-ins haven't changed:

(In reply to Stuart Morgan from comment #0)
> system; failing that maybe we'll delete pluginreg.dat on the first quit
> after each update, so the second launch will rebuild (although I'd like to
> avoid it if we did an autoupdate and deleted it *before* the update already)

Maybe after getting 2.1.1 out the door I'll have time to do some debugging and see how our camino.disabled_plugin_names makes Core learn about changes and how that differs from the "actual" blocklisting.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> Maybe after getting 2.1.1 out the door I'll have time to do some debugging
> and see how our camino.disabled_plugin_names makes Core learn about changes
> and how that differs from the "actual" blocklisting.

Since I still have the NSLog in from trying to determine if my code is working, I see that we're getting asked about the plug-in each time an instance is started, so something is definitely calling our PluginBlocklistService::GetPluginBlocklistState implementation often.

As soon as I add the plug-in name to "camino.disabled_plugin_names", there's never another call.  There's not a call when I flip the pref (whether there's plug-in content on current pages or not), there's not a call when I load a new page, nothing; the plug-in is just disabled.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Partial fix (v0.5)

This sort-of works.

I realized that PreferenceManager's updatePluginEnableState twiddles a bit directly on the pluginTag to turn the plugin off.  Turns out you can also directly twiddle a blocklisted bit, too.

If you just twiddle blocklisted, the plugin starts to load and then "goes away" leaving an empty space (the same thing that happens to a running plugin when you add it to camino_disabled_plugin_names); a reload will then show the "blocked" placeholder.

If you twiddle both blocklisted and disabled bits, it (at least on http://www.adobe.com/software/flash/about/) prevents the start-to-load scenario, but you still need a reload load to see the placeholder.  SetBlocklisted is supposed to set the disabled itself, though…

The other problem is that once we've set the state, we don't get asked again, so if the user during the session sets the dangerous plugin pref during the session, it won't enable said dangerous plugin.  Maybe we're OK with this, since people now (when the blocklist actually works) have to quit and trash pluginreg.dat; otherwise, we could probably do the notification dance.

I'm reasonably convinced that Firefox is doing roughly the same thing in http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/extensions/src/nsBlocklistService.js#797-829.

I don't know if we can improve the "no notification on first plug-in instance" case by having PreferenceManager call GetPluginBlocklistState (or some wrapper or helper) early enough in startup that we have a pluginHost but no pages/pluginInstances (does Gecko read the plug-in state before consulting the blocklist on every load?), or if Gecko would still have to load once to check the flags.  Still, this is better than "may not ever work" situation I'm currently experiencing!

---

Also, regarding our current hack of deleting pluginreg.dat on autoupdate, do you think that (new-in-1.9.2) pluginHost's reloadPlugins might work? http://mxr.mozilla.org/mozilla1.9.2/source/modules/plugin/base/public/nsIPluginHost.idl#77  Er, maybe it's only "new to nsIPluginHost.idl" in 1.9.2 and it used to live elsewhere…?

Comment 10

[Stuart Morgan]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #7)
> Since I was testing with a fresh profile, I should have had a completely
> fresh pluginreg.dat getting built and consulting our blocklist, but Flash
> wasn't being blocked.  I launched again with the same profile, and still no
> luck.  I deleted pluginreg.dat and restarted again with the same profile,
> still not blocked.

This makes no sense at all to me., pluginreg.dat is where all the plugin info is stored, so I don't see how deleting pluginreg.dat could not cause a from-scratch re-scan, which should call our code. I'd be curious to know, when you get a chance, what happens for you in nsPluginHost::ReloadPlugins on startup after nuking that file, especially whether mPluginsLoaded is true. And I guess also what nsPluginHost::ReadPluginInfo() does on startup when you've nuked the file.

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #9)
> Also, regarding our current hack of deleting pluginreg.dat on autoupdate, do
> you think that (new-in-1.9.2) pluginHost's reloadPlugins might work?

Alas, no. nsPluginHost::ReloadPlugins, which is what backs it, is the tempting method I first looked at, only to discover that it means "re-scan the disk and compare to the cache to see if anything is changed". It's exactly the code that's run at startup and doesn't work correctly because it only uses top-level timestamps.

(Josh mentioned to me at some point that the extreme brokenness of detecting plugin changes on Mac is fixed on trunk, but sadly that's unlikely to help since my impression is that a whole lot has been rewritten, so I highly doubt it's feasibly backportable.)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #10)
> This makes no sense at all to me., pluginreg.dat is where all the plugin
> info is stored, so I don't see how deleting pluginreg.dat could not cause a
> from-scratch re-scan, which should call our code. I'd be curious to know,
> when you get a chance, what happens for you in nsPluginHost::ReloadPlugins
> on startup after nuking that file, especially whether mPluginsLoaded is
> true.

According to my gdb breakpoint, nsPluginHost::ReloadPlugins never gets called on startup when pluginreg.dat is not present (neither on a fresh profile, nor after deleting an existing pluginreg.dat).

(Incidentally, when running a build with bug 727667 and Flash 10.3.183.11, loading some Flash content, and quitting, the interesting entries for Flash--the fields after the timestamp on that line--are the same for Flash as for other plug-ins, so somehow the blocked state isn't even getting written!)

> And I guess also what nsPluginHost::ReadPluginInfo() does on startup
> when you've nuked the file.

Some more pointers on what to look for her would be helpful for when I do have time to dig into this ;)

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Useless gdb logging

I wanted to see who was calling our PluginBlocklistService::GetPluginBlocklistState impl, and it's nsObjectLoadingContent::Instantiate http://mxr.mozilla.org/mozilla1.9.2/source/content/base/src/nsObjectLoadingContent.cpp#1915

Unfortunately, that method only seems to care about STATE_OUTDATED, which is why loading a page with a blocked plugin does absolutely nothing.

(I ran this test three times: creating a fresh profile [so no existing pluginreg.dat], after the first run [so pluginreg.dat that was created during first run], and after deleting the pluginreg.dat at the end of the second run [so no pluginreg.dat]).

What I still don't understand is why we aren't hitting the call to the blocklist inside ScanPluginsDirectory (which method is eventually called by everything that wants to load, reload, enumerate, or get plug-ins) on startup, when I can clearly see plug-ins being loaded due to the "[loaded plugin /path/to/foo.plugin]" logspam…

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: More gdb logging

Oh, this is interesting.

On a fresh profile, with breakpoints at 

> Breakpoint 1: nsPluginHost::ScanPluginsDirectory
> Breakpoint 2: PluginBlocklistService.mm:117 (our PluginBlocklistService::GetPluginBlocklistState impl)
> Breakpoint 3: nsPluginHost.cpp:4775 (call from inside nsPluginHost::ScanPluginsDirectory to PluginBlocklistService::GetPluginBlocklistState)

We never hit breakpoints 2 or 3, while we hit breakpoint 1 three times (I guess once for every plug-in directory that's defined: the app, the user Library, and the Library--I don't think our ~/Library/Application Support/Camino/Internet Plug-Ins is defined or exists yet.

And, this happens really early, in PreferenceManager's syncMozillaPrefs, which is probably before we have the Blocklist Service…

Yup, if I also break on http://mxr.mozilla.org/mozilla1.9.2/source/modules/plugin/base/src/nsPluginHost.cpp#4773, I hit the breakpoint after hitting nsPluginHost::ScanPluginsDirectory the second time, and p (bool)blocklist gives false, so we have no Blocklist Service at the only point in time at which it matters :-(

So, at the very least we need to initialize our blocklist service implementation before we do any plug-in-related pref setup (in this case, seeing if we should load the Flashblock stylesheet) to make the the blocklist service work *at all*.

That may end up fixing the bug entirely; I guess we'd have to see.  But if it doesn't, we maybe should still try pushing the blocked state to PluginHost to work around stale/undeleted pluginreg.dat in something like attachment 597699 [details] [diff] [review].

The (extremely!) pressing question is, then, how do initialize our blocklist service implementation before we do any plug-in-related pref setup?

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, it looks like the Firefox version used to be "loaded" (whatever that means) at startup by registering for "profile-after-change" and "update-timer"; bug 532552 changed it to only register for "update-timer" (http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b343b3557268).

Is there anything that tells non-JS services when to load, or do they just load when first called (in which case, why is that not happening for us)?

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, this should be registered (or whatever) via AppComponents, via the CHBrowserService::RegisterAppComponents call in MainController.

Ohhhhh.

Is this the problem?

MainController.mm:

188 - (void)ensureGeckoInitted
189 {
190   if (mGeckoInitted)
191     return;
192 
193   // bring up prefs manager (which inits gecko)
194   [PreferenceManager sharedInstance];
195 
196   // register our app components with the embed layer
197   unsigned int numComps = 0;
198   const nsModuleComponentInfo* comps = GetAppComponents(&numComps);
199   CHBrowserService::RegisterAppComponents(comps, numComps);
200 

So [PM sharedInstance] brings up Gecko, but also (in prefs setup) causes us to trigger the Gecko plug-in scanning calls which want the Blocklist Service.  And then, once that's all done, we finally register our app components (including the Blocklist Service) with Gecko :-(

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(Also, for our other components, we appear to use the same CID as the default Gecko component we're (overriding|implementing), but for the blocklist, we use a different CID than nsBlocklistService.js.  Why is that?)

[1] http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/mozapps/extensions/src/nsBlocklistService.js#866
[2] http://mxr.mozilla.org/camino/source/camino/src/application/PluginBlocklistService.h#55

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Make blocklist registration happen before we cause plug-in enumeration

Stuart, what do you think about this fix to get blocklisting working again?

I looked through syncMozillaPrefs and the Get/RegisterAppComponents stuff, and it doesn't seem like there's anything in the latter that depends on the former having run first (although my eye is probably not the best judge of that).

I've run a quick test and confirmed that blocklisting does indeed work again on a fresh profile, and nothing seems obviously broken (although Flasblock seems to take precedence over blocklisting; did that happen back when blocklisting still worked before--possibly as late as bug 662666, but definitely at the time of bug 626477?), but more testing would be useful.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #17)
> on a fresh profile, and nothing seems obviously broken (although Flasblock
> seems to take precedence over blocklisting; did that happen back when
> blocklisting still worked before--possibly as late as bug 662666, but
> definitely at the time of bug 626477?), but more testing would be useful.

Wait, that doesn't make sense at all.  Post-bug 665550, we should only be allowing Flashblock if Flash is installed, and for some definition of "installed", blocklisted plug-ins aren't "installed" (e.g., they do not show in about:plugins).

Oh, no, isPluginInstalledForType: only cares about the failure case from nsPluginHost::IsPluginEnabledForType, meaning no plug-in is physically present, which I think is ultimately the right check for this case.

And, yes, to answer my earlier question, back at the time of bug 626477, Flashblock was allowed to take precedence over blocklisting (although at least at adobe.com clicking the Flashblock placeholder reveals the blocklist placeholder, yay!).

It seems a little weird allowing it to be active when Flash is blocklisted, but a) that's definitely something for another bug and b) I'm not sure I care enough to jump through all the hoops that would be necessary to code a nice solution to the problem (similar to what we had to write for when a user had Flashblock enabled and then turned off one of its deps, JS or plug-ins).

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

More than half of this bug is now describing the problem succinctly outlined in the last paragraph of comment 13, so I've spun the original bug (find some way of better forcing plugin state updates) off into bug 740620 and refocused this bug on the "blocklist isn't working!" problem.

Comment 20

[philippe (part-time)]

Comment on attachment 610668 [details] [diff] [review]
Make blocklist registration happen before we cause plug-in enumeration

I'm probably not the best person to report on plugin behavior…, but anyway.
A patched build seems to work fine with Flash 10.1.3.18 (latest up to date) – tests with the Adobe test page and a youtube video, MS Silverlight demos (with Silverlight 5 installed), Flip4mac 2.4.1.4 (tested against the url in bug 579996, one of the few places I found to still call F4M). Flash is all very basic testing - the YT player control bar works. My 10years old tester is away 'till next week.

On the 10.7 machine (which hasn't seen a Flash plugin for 18months), installing flash 10.0.45.2 and running a patched build: Flash is blocked and not listed in about plugins (tested on a default profile, TrC).

On the 10.6 machine, a test with Flash 10.0.45, installed in ~/library/application support/camino/internet plugins (Flash normally not available on that account). First ran Camino 2.1.2ml, plugin recognized in about:plugins, and loads on the adobe test page. Then a patched Camino, same profile, I had to throw away the pluginreg.date file for the plugin blocking to work (?). If the plugin is installed in ~/library/internet plugins, and testing with TrC, it is blocked on the adobe test page, and not listed in about plugins.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to philippe from comment #20)
> I'm probably not the best person to report on plugin behavior…, but anyway.

Err, sorry, I wasn't clear :-( I'm happy you did the plug-in testing (see below), but I was actually most interested in your amazing ability to find other problems that crop up ;-) in this case potentially from the slight change in the order we run our Gecko pref setup vs. registering app components.  I don't expect any problems, but if you could use a patched build for a bit and watch for any strangeness, that would be helpful.

> On the 10.6 machine, a test with Flash 10.0.45, installed in
> ~/library/application support/camino/internet plugins (Flash normally not
> available on that account). First ran Camino 2.1.2ml, plugin recognized in
> about:plugins, and loads on the adobe test page. Then a patched Camino, same
> profile, I had to throw away the pluginreg.date file for the plugin blocking
> to work (?).

That's "expected" behavior because I believe that's one of the manifestations of the problem that's now tracked as bug 740620.  (My patch on that bug should "fix" the scenario you described, with the caveat that the first plug-in instance of a blocked plug-in will go blank instead of showing a placeholder.)

Comment 22

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #21)
> (In reply to philippe from comment #20)
> > I'm probably not the best person to report on plugin behavior…, but anyway.
> 
> Err, sorry, I wasn't clear :-( I'm happy you did the plug-in testing (see
> below), but I was actually most interested in your amazing ability to find
> other problems that crop up ;-) in this case potentially from the slight
> change in the order we run our Gecko pref setup vs. registering app
> components.  I don't expect any problems, but if you could use a patched
> build for a bit and watch for any strangeness, that would be helpful.

That patched build has been running on 10.7 for a few hours and so far I haven't seen anything to worry about. I'll verify on 10.6 tomorrow.

Comment 23

[Stuart Morgan]

Ideally we should not have all the critical core bringup buried in the pref class :P That's a much bigger fish though.

Given that we do, how about we just move the component registration into the initMozillaPrefs function where everything else important happens?

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Move component registration into initMozillaPrefs (doesn't work)

(In reply to Stuart Morgan from comment #23)
> Ideally we should not have all the critical core bringup buried in the pref
> class :P That's a much bigger fish though.

I had wondered about the reasoning behind that architectural decision, too.

> Given that we do, how about we just move the component registration into the
> initMozillaPrefs function where everything else important happens?

That doesn't appear to work, at least not with a simple "move stuff from one file to another" fix.  I don't have time tonight to move stuff around within initMozillaPrefs and play around with gdb (I suspect it might have to do with the embedding-initialized notification, but I don't know).

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #24)
> Created attachment 611336 [details] [diff] [review]
> Move component registration into initMozillaPrefs (doesn't work)
> > Given that we do, how about we just move the component registration into the
> > initMozillaPrefs function where everything else important happens?
> 
> That doesn't appear to work, at least not with a simple "move stuff from one
> file to another" fix.  I don't have time tonight to move stuff around within
> initMozillaPrefs and play around with gdb (I suspect it might have to do
> with the embedding-initialized notification, but I don't know).

I eventually moved component registration (and syncMozillaPrefs) all the way to the end of initMozillaPrefs, just before returning YES, and blocklisting still doesn't work:

[Embedding Notification];
[Component Registration Goop];
[self syncMozillaPrefs];
return YES;

I have absolutely no idea why.

I've confirmed component registration happens.  I've even confirmed that we have a blocklist (breakpoints 2 and 3 in comment 13 are being hit, and 'p (bool)blocklist' returns '192' instead of 'false'), but for whatever reason, on a fresh profile, we still fail to blocklist outdated Flash using this configuration, whereas things work fine when we keep component registration in MainController and rearrange syncMozillaPrefs in the first patch.

Stuart, can you suggest anything to investigate why/how this is failing, or should we just take the first patch instead?

Comment 26

[Stuart Morgan]

My guess from looking at the code is an issue in the ordering of writing the compatibility file and registering components, but I haven't verified that.

How about this: I really don't want to move syncMozillaPrefs out, because that's one of the few things that actually makes sense where it is in this whole flow. But the actual problem is the flash call in the stylesheet block, right? Let's break that whole section of syncMozillaPrefs out into a loadUserStylesheets method, and have MC call that just after component registry (vs. moving all of syncMozillaPrefs). That code doesn't quite belong in the method it's in anyway.

Comment 27

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The actual problem is anything that can cause plug-in enumeration in syncMozillaPrefs.  The Flash check in the stylesheet loading will cause it, as will calling [self updatePluginEnableState] in the Java pref migration.  

However, for 90+% of users, the Java pref migration has already run, so moving the stylesheet loading should solve the problem with minimum of fuss, and work right on the second launch for those people. (And that only runs if mLastRunPrefsVersion, so fresh profile testing will be OK.)

I'll whip up that patch and make sure it works…

Comment 28

[Stuart Morgan]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #27)
> as will calling [self updatePluginEnableState] in the Java pref migration.  

Oops, missed that when I was scanning for plugin usage in pref startup. I'm fine with that case being broken though, since as you point out it's a 0-to-1-time problem.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Move stylesheet loading out of syncMozillaPrefs and after component registration

Yeah, breaking the stylesheet loading out of syncMozillaPrefs works fine (with the expected caveat that the late-upgrader-Java-disabling 1% case will need a fix for bug 740620 or some other condition that causes Gecko to re-scan the plug-ins folders).

I may have gone comment-happy as a result of all the hours and lost hair this bug has caused ;-)  I'm not sure which place(s) are the best for the explanatory comments, so please advise on that.

Comment 30

[Stuart Morgan]

Comment on attachment 618553 [details] [diff] [review]
Move stylesheet loading out of syncMozillaPrefs and after component registration

sr=smorgan

Heavy commenting is fine; I would just change the one on loadUserStylesheets to be less specific and say "Must be called after component registration, since it accesses the plugin list."

Comment 31

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/a45013d43060 with the comment for loadUserStylesheets tidied up.

Based on all of our analysis, I'm pretty sure I caused this bug by fixing bug 665550 for cl :-(  I'm not sure the added comments would have stopped me from doing that, but maybe.