Closed Bug 632261 Opened 12 years ago Closed 12 years ago

Allow users to override ad-blocking rules without hacking the bundle

Categories

(Camino Graveyard :: Annoyance Blocking, enhancement)

Product:
Camino Graveyard
Component:
Annoyance Blocking
Platform:
All
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: alqahira, Assigned: alqahira)

References

Details

Attachments

(1 file, 1 obsolete file)

Stab #2
22.02 KB, patch
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review 
Currently, if you want to override (or supplement) our ad-blocking rules (because one obscure site you frequent uses some dumb "/ad/" pattern for required content, for instance), you have two options, neither very pretty:

1) Use userContent.css, which is always loaded, unilaterally, at startup (so you can't turn it off ever, nor can you add rules on the fly)

2) Hack ad_blocking.css in the bundle, which requires you to touch the bundle (ew), and requires you to re-do your modifications every time you update.

My solution is this:

1) Have PreferenceManager's |refreshAdBlockingStyleSheet:| load ad_blocking_sheets_loader.css, whose contents are:

@import url(ad_blocking.css);
@import url("chrome://global/content/cm_user_ads.css");

This gets the user's sheet/rules loaded after ours, so our rules can be overridden.

2) Register chrome from the profile's chrome directory

3) (Optionally,)
  a) ship a chrome.manifest that resolves 
     chrome://global/content/cm_user_ads.css to a local file in the profile 
     chrome folder
  b) ship a dummy local file to correspond to the local file in a) above
  c) have our profile code create the chrome folder and populate it with files
     from a) and b)

2 is required because the toolkit chrome registry doesn't register chrome from the profile chrome folder (only nsLayoutStylesheetCache.cpp and profile migrators use NS_APP_USER_CHROME_DIR), whereas the old chrome registry itself registered chrome from NS_APP_USER_CHROME_DIR (in nsChromeRegistry.cpp's nsChromeRegistry::LoadProfileDataSource()).

3 is suggested because it prevents nsCSSLoader.cpp spewing an assertion about the file being missing (and something in the chrome system whining, too; it doesn't provide a line number); we'd have to implement it ourselves because, once again, the old chrome registry implemented (in nsChromeRegistry.cpp's nsChromeRegistry::GetProfileRoot) making the folder and copying userContent/userChrome over, and the new toolkit chrome registry (surprise, surprise) does not.
(In reply to comment #0)
> 3) (Optionally,)
>   a) ship a chrome.manifest that resolves 

After some more thought, I think we want to give this a name like "ad_blocking.manifest" instead, so that anyone dumping an extension's chrome.manifest into the profile chrome folder doesn't accidentally stomp our file.
Attached patch First stab at complete fix (obsolete) — — Splinter Review 
Here's the fix for this bug.  This patch depends on the patch for bug 632269 being applied[1].

The code to do the folder-creation and folder-item-population is probably a little hacky; here are some of the things I'd like to know if there's a better solution for:

1) Bug 632269's
  rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
                              getter_AddRefs(appUserChromeDir));
  NS_ENSURE_SUCCESS(rv, rv);
code is always going to return failure on that rv check on the first launch of a new profile, because $profile/chrome does not yet exist :(

2) Related, I tried to weave creation into something like |AppDirServiceProvider::GetFile| and have it call a theoretical |GetProfileChromeDirectory| when asked for NS_APP_USER_CHROME_DIR, but no-one asks for that folder until we get into the code from bug 632269 :(

3) Ideally, we'd only try to run the folder creation code if we know it's needed, but my attempts to short-circuit that before calling |EnsureExists| failed.

4) To do the file-population, I copied the relevant method from nsProfileDirServiceProvider.cpp; I just shoved it into our file as-is.

5) It seems kind of icky to have the folder-creation and file-population code live in the the chrome manifest directories enumeration, but I wasn't sure where else to put it given 2), and given that for the purposes of this bug, we want to the folder and files to exist prior to enumerating the chrome manifest directories and registering chrome therein.

6) It also seems kind of icky that we have AppDirServiceProvider knowing about the profile chrome files for this bug, but again I don't know how else to get them in place prior to chrome registration; I don't know what the various interactions between/relative timestamps of PreferenceManager's |initMozillaPrefs| and chrome registration are.

7) I'm not sure if this will ever be a problem, and if it will be whether it's worth worrying about, but there's not really a sane way for us to update either profile.manifest or user_ad_blocking.css once we've created them in the profile (at least not, I think, before chrome registration occurs?).  Hopefully we'll never need to update them?

8) Needless to say, all the GeckXPCOM C++ that I "wrote" myself for the patch was a compare/copy/paste/compile job, so please correct ;)

I'm not sure if we can fix 1), 2), and 5) by refactoring some things somewhere; I'm open to suggestions of how best to handle that.

philippe, can you make sure my stylesheet cascade/inheritance stuff is right?

[1] By itself, that patch spams some chrome registry and localFile NS_ENSURE_SUCCESS failures on startup if there's no "chrome" folder in the profile; I think the failures are harmless, but I'd rather wait and land that patch at the same time as code that creates that folder.
Attachment #513939 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #513939 - Flags: review?(phiw)
Comment on attachment 513939 [details] [diff] [review]
First stab at complete fix

(In reply to comment #2)
 
> philippe, can you make sure my stylesheet cascade/inheritance stuff is right?

Your set up will work fine, the selectors in the user stylesheet will be counted as coming after the ones in ad_blocking.css and override them. There might eventually be a rare case where the user needs some additional specificity, but that shouldn't be difficult to accomplish. Our ad_blocking selectors have fairly low specificity in general.

I tested this both with my normal profile and with a new profile, and in both cases everything worked fine; editing user_ad_blocking.css and then refreshing via the preference pane works smoothly.
Attachment #513939 - Flags: review?(phiw) → review+
Comment on attachment 513939 [details] [diff] [review]
First stab at complete fix

Stuart, any idea when you might get to this?

This is the single most annoying thing for me every time I update my nightly ;-)
Sorry, I totally forgot about this. I'll take a look at the questions above sometime this week if at all possible.
The parts you've labeled as icky make me really sad. Can't we instead:
1) Fix bug 632269 so that it only adds the user chrome dir to the listing if the folder exists (thus getting rid of the assertion failures there)
2) Change PreferenceManger.mm to register/unregister a user style sheet at a known location whenever the pref toggles (just as we do with the main ad-block sheet) if that file exists.
It's been a long time now since I had all of the understanding this (and related chrome, toolkit, and dirserviceprovider) code in my head, but a couple of comments from stale memories and fresh eyes:

(In reply to comment #6)
> The parts you've labeled as icky make me really sad. Can't we instead:
> 1) Fix bug 632269 so that it only adds the user chrome dir to the listing if
> the folder exists (thus getting rid of the assertion failures there)

The folder really should exist (and does in all profiles created by 2.0.x and older, because the old chrome registry created it; in other Gecko apps, XREDirProvider creates it).

So I think somehow before we register we need to do something like XREDirProvier's |nsXREDirProvider::EnsureDirectoryExists| (or the old chrome registry's |nsChromeRegistry::GetProfileRoot| if exists code).  I think that's possible.

In fact, can I just do 

  rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
                              getter_AddRefs(appUserChromeDir));
+ rv = EnsureExists(appUserChromeDir);
  NS_ENSURE_SUCCESS(rv, rv);

like you did in |AppDirServiceProvider::GetProfilePluginsDirectory| ?

If that works, that should make that better and probably start making the ball rolling.

Then most of 5 *ought* to be able to be ameliorated by moving the manifest and stylesheet creation into |AppDirServiceProvider::GetFile| (which chrome enumeration should trigger?, possibly also with EnsureExists, either in enumeration or in GetFile).

Not sure; I'll have to dig back in.

> 2) Change PreferenceManger.mm to register/unregister a user style sheet at a
> known location whenever the pref toggles (just as we do with the main
> ad-block sheet) if that file exists.

That won't fix this bug, because stylesheet loading is indeterminate: http://mxr.mozilla.org/mozilla1.9.2/source/layout/base/nsIStyleSheetService.idl#67

Thus, to allow a user to _override_ our rules, their rules have to come last in the cascade.  To make that happen--aside from I suppose generating ad_blocking_loader.css on the fly, every launch of the app, with the absolute path to the user's profile folder--the only way to load a user-controlled file from outside the bundle is by giving said file a chrome URL.
(In reply to comment #7)
> (In reply to comment #6)
> > The parts you've labeled as icky make me really sad. Can't we instead:
> > 1) Fix bug 632269 so that it only adds the user chrome dir to the listing if
> > the folder exists (thus getting rid of the assertion failures there)
> 
> The folder really should exist (and does in all profiles created by 2.0.x
> and older, because the old chrome registry created it; in other Gecko apps,
> XREDirProvider creates it).
> 
> So I think somehow before we register we need to do something like
> XREDirProvier's |nsXREDirProvider::EnsureDirectoryExists| (or the old chrome
> registry's |nsChromeRegistry::GetProfileRoot| if exists code).  I think
> that's possible.
> 
> In fact, can I just do 
> 
>   rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
>                               getter_AddRefs(appUserChromeDir));
> + rv = EnsureExists(appUserChromeDir);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> like you did in |AppDirServiceProvider::GetProfilePluginsDirectory| ?

Well, never mind the fact that I did it a few lines below my quoted lines, after transforming it from an nsIFile into an nsILocalFile so that EnsureExists would work.  

However, as far as I can tell right now/so far (that is, testing with just the patch for bug 632269 + the transforming for EnsureExists, and then with a stock AppDirServiceProvider with some logging), the NS_ENSURE_SUCCESS(rv, rv); failure is coming from somewhere else (possibly someone looking for NS_APP_USER_PROFILES_ROOT_DIR).

I see one failure with the patch+creation in and one failure with the patch+creation out; it appears in the logging in this context:

GetFile called on DefProfRt
AppDirServiceProvider::GetProfileDirectory called
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520006: file /Users/smokey/Camino/dev/192branch/mozilla/xpcom/io/nsLocalFileOSX.mm, line 392

That's "NS_ERROR_FILE_TARGET_DOES_NOT_EXIST (0x80520006)", according to https://developer.mozilla.org/en/Table_Of_Errors#File_Errors

I know the logging is not always synchronous and in the right order; is there any way I can break on that line in nsLocalFileOSX.mm only when it fails, and bt from there to see for sure who triggers it (e.g., both right now and when I put the full patch from this bug in), to make sure it's not my code?

> Then most of 5 *ought* to be able to be ameliorated by moving the manifest
> and stylesheet creation into |AppDirServiceProvider::GetFile| (which chrome
> enumeration should trigger?, possibly also with EnsureExists, either in
> enumeration or in GetFile).

This comment is at least partially wrong, too :P  I added |AppDirServiceProvider::EnsureProfileFileExists| to handle the if-exists stuff, but I do think |AppDirServiceProvider::GetFile| should have entries for these new properties, and that's where the |AppDirServiceProvider::EnsureProfileFileExists| calls should be, and that would remove the ickyness of item 5, if the plan works.
(In reply to comment #7)
> The folder really should exist (and does in all profiles created by 2.0.x
> and older, because the old chrome registry created it; in other Gecko apps,
> XREDirProvider creates it).

I see; I didn't realize we explicitly wanted it to be auto-created (vs. that just being a means the end of fixing the check failure)

> That won't fix this bug, because stylesheet loading is indeterminate:
> http://mxr.mozilla.org/mozilla1.9.2/source/layout/base/nsIStyleSheetService.
> idl#67

Hrm. Could we make the ad-blocking sheet an agent sheet instead of a user sheet? Do we have any order dependencies with the aqua or flashblock styles?
(In reply to comment #8)
> I know the logging is not always synchronous and in the right order; is
> there any way I can break on that line in nsLocalFileOSX.mm only when it
> fails, and bt from there to see for sure who triggers it (e.g., both right
> now and when I put the full patch from this bug in), to make sure it's not
> my code?

You should be able to set the breakpoint and then do
cond <#> rv!=0
where <#> is the number of the breakpoint you set.
(In reply to comment #7)
> In fact, can I just do 
> 
>   rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
>                               getter_AddRefs(appUserChromeDir));
> + rv = EnsureExists(appUserChromeDir);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> like you did in |AppDirServiceProvider::GetProfilePluginsDirectory| ?

Yes, except you also need an NS_ENSURE_SUCCESS in between (unless I'm confused, NS_GSD only fails if it can't make the path; it shouldn't matter if the directory doesn't exist; if I am confused, then instead do NS_ENSURE_TRUE(appUserChromeDir, NS_ERROR_FAILURE) between them)
(In reply to comment #10)
> (In reply to comment #8)
> > I know the logging is not always synchronous and in the right order; is
> > there any way I can break on that line in nsLocalFileOSX.mm only when it
> > fails, and bt from there to see for sure who triggers it (e.g., both right
> > now and when I put the full patch from this bug in), to make sure it's not
> > my code?
> 
> You should be able to set the breakpoint and then do
> cond <#> rv!=0
> where <#> is the number of the breakpoint you set.

That didn't work :(

gdb complained "Error parsing breakpoint condition.  Will try again when we hit the breakpoint." and broke every time I hit line 392.

When I tried to set the breakpoint on 393 (because I have vague recollections of you and Wevah telling me one time that the values of the variables aren't known in gdb at the line where they're used, since it breaks first), gdb set the breakpoint for 394 but liked the cond 1 rv!=0 syntax.  However, it failed to ever break, and I saw the failure fly by in the logspam :(
Hm, well if NS_ENSURE_SUCCESS is line 392, then rv should already be set there. Maybe it's the fact that it's a macro that's causing issues.

Is this http://mxr.mozilla.org/mozilla1.9.2/source/xpcom/io/nsLocalFileOSX.mm#392 or are you not synced with that file? If it is, try the break on 394; 393 is a blank line.

I'll try debugging here if I can find time.
(In reply to comment #13)
> Hm, well if NS_ENSURE_SUCCESS is line 392, then rv should already be set
> there. Maybe it's the fact that it's a macro that's causing issues.
> 
> Is this
> http://mxr.mozilla.org/mozilla1.9.2/source/xpcom/io/nsLocalFileOSX.mm#392 or
> are you not synced with that file? If it is, try the break on 394; 393 is a
> blank line.

That file hasn't changed since 2009, so I'm up-to-date.  Breaking on 394 had the same result as breaking on 393; gdb liked the cond but never hit the breakpoint, and I saw the failure fly by in the logspam.

> I'll try debugging here if I can find time.

Given that it's now clear that the failure happens in stock Camino, it's definitely not caused by my patch as I had thought back in comment 2 point 1, so it's less important unless I suddenly start seeing more of them :P
Attached patch Stab #2 — — Splinter Review 
Here's the next stab at this, with some refactoring (and, because of which, this patch now subsumes bug 632269; I could re-break it out, but that's probably counterproductive at least at this time).  There are now fewer icky things, which hopefully will make Stuart less sad.

In terms of the points I raised in comment 2:

1) Isn't related to this patch (or bug 632269) at all

2) See point 5.

3) I'm not sure really is relevant; |EnsureExists| either ensures the folder exists or creates it if it doesn't.

4) No change, but unless there's a better way to do this, it's not really an issue.

5) This patch refactors profile chrome folder creation and profile chrome folder files creation out of the chrome manifest directories enumeration; yay!  There's a long, smichaudesque comment of which I'm not proud in the middle of the new |GetProfileChromeDirectory| explaining why the two parts can't be separated[1]; that can perhaps be trimmed or removed entirely, but I wanted to get things down as I was writing the code, so I wouldn't forget if too much time passed.

[1] Actually, we could have two methods, one that just gets/creates the folder, and one that first calls the get/create folder method and then does the file copying, and have the chrome manifest directories enumeration code call the second method, but that seems like churn for little benefit--we'd have to duplicate some of the GeckXPCOM junk in both methods, and the chrome directory enumeration code would be calling |GetProfileChromeDirectoryAndCreateUserChromeFiles|, which puts it out of sync with the other locations which are just fetching the folder.  So, I think the present refactoring is the extent that makes sense. 

6) This is mitigated somewhat by the fix for point 5, but I don't think there's any way around this given the need to beat chrome registration.

7) Now that creation is refactored, I think in the unlikely event we ever need to do something, we 1) change the name of the new/updated files and 2) add code to |GetProfileChromeDirectory| to delete the old files, and we'll beat chrome registration and all will be well.

8) My understanding of GeckXPCOM C++ has not improved since the last patch ;-)

In terms of new questions, I really only have one: 

9) Is the |return| from |GetProfileChromeDirectory| correct?  Initially I just had the method |return rv| after 
  rv = EnsureExists(appUserChromeDirLocalFile);
  NS_ENSURE_SUCCESS(rv, rv);
(this was before I'd hooked up the file-creation code, so the method really did end at that line at that point in time), but when I did so, the chrome registry complained that I was feeding it a non-nsILocalFile ("###!!! ASSERTION: Directory enumerator returned a non-nsILocalFile: 'Error', file /Users/smokey/Camino/dev/192branch/mozilla/chrome/src/nsChromeRegistry.cpp, line 1179").

So I then looked around for other localFile-returning methods and copied the code in |GetSystemDirectory|, which looked like it was doing the same sort of thing as I needed to do here.

Regardless of whether we can/should have PreferenceManager register a different level of style sheet, bug 632269 is going to need most of the AppDirServiceProvider changes.

And on that point, I imagine smfr chose user sheet for a reason (perhaps because UA sheets can do unsafe things and cause crashes, per https://developer.mozilla.org/en/Using_the_Stylesheet_Service#Using_the_API ?)

In theory I suppose there is an order dependency between flashblock and ad_blocking (i.e., bug 384729), but I'm not sure whether it's worth the time, if it's even possible, to unravel.

I don't have a solid enough command of CSS cascade and specificity to comment on the aquaSelect case; I'd guess we'd want that to be a user sheet to be able to trump author styles, but I don't know.
Attachment #513939 - Attachment is obsolete: true
Attachment #543351 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #513939 - Flags: superreview?(stuart.morgan+bugzilla)
(In reply to comment #9)

> Hrm. Could we make the ad-blocking sheet an agent sheet instead of a user
> sheet? Do we have any order dependencies with the aqua or flashblock styles?

AquaSelect, Flashblock stylesheets are unaffected by and independent from ad-blocking stylesheet(s). Well, there is some code in ad_blocking to attempt to override the Flashblock stylesheet (as noted in last paragraph of comment 15), but it doesn't really work.

I don't think making ad_blocking an agent sheet will cause much problems, as far as that stylesheet interacts with content. If the user edits that stylesheet, then yeah... unsafe things e.a.

However, and more importantly for the purpose of the discussion here, a user_ad_blocking sheet would have to be an agent sheet as well to be able to override the default ad_blocking. !important in agent stylesheets is near impossible to override, even at the user stylesheet level.
(authors have long been pissed at the impossibility to override some of the forms.css rules due to !important – it is not better for user stylesheets).

Can Gecko load an agent stylesheet from <profile location>/chrome ?
(In reply to comment #16)
> However, and more importantly for the purpose of the discussion here, a
> user_ad_blocking sheet would have to be an agent sheet as well to be able to
> override the default ad_blocking. !important in agent stylesheets is near
> impossible to override, even at the user stylesheet level.

And making both agent stylesheets would then bring us back to the indeterminate load order problem, which means we'd still have to do some explicit ordering, which means we'd need to do this bug to control ordering, and we may as well just keep them both user sheets.

> Can Gecko load an agent stylesheet from <profile location>/chrome ?

The stylesheet service MDC doc doesn't mention any restrictions, but it sounds like that's not going to matter here anyway.
(In reply to comment #17)
 
> And making both agent stylesheets would then bring us back to the
> indeterminate load order problem, which means we'd still have to do some
> explicit ordering, which means we'd need to do this bug to control ordering,
> and we may as well just keep them both user sheets.

Hmm, hold on, maybe I'm a little bit confused here as to what exactly the problem is… Given the way the stylesheets are set up (in ad_blocking_loader), Gecko, at parse time, will always count user_ad_blocking /after/ ad_blocking. That is how the cascade works. Thus given the exact same specificity for a given selector in both stylesheets, the one in user_ad_blocking will win, always. 'source order' does matter here.
(In reply to comment #18)
> Hmm, hold on, maybe I'm a little bit confused here as to what exactly the
> problem is… Given the way the stylesheets are set up (in
> ad_blocking_loader), Gecko, at parse time, will always count
> user_ad_blocking /after/ ad_blocking. That is how the cascade works. Thus
> given the exact same specificity for a given selector in both stylesheets,
> the one in user_ad_blocking will win, always. 'source order' does matter
> here.

In the alternative scenario (the one that would involve agent vs user sheets), we wouldn't be using ad_blocking_loader at all; PreferenceManager would just have the stylesheet service load two sheets, Camino.app/Contents/Resources/ad_blocking.css and $profile/user_ad_blocking.css.  In that case, load order would be indeterminate, but we would have *some* control over the cascade if we made one an agent sheet and one a user sheet; however, that takes us back to your conclusions in comment 16.

In the scenario in my patch, we can and do control load order, as you noted, by having the stylesheet service load only ad_blocking_loader, which @imports the other two sheets in the appropriate order.  We just have to give the user_ad_blocking sheet a chrome URL so that we can reference a profile-located file (whose path is different per-user and might change per-profile, e.g. Portable Camino) in the loader sheet, and that involves some poking of chrome registration.  Stuart just wanted to see if there was a viable alternative to doing that and presented the alternate scenario.
Comment on attachment 543351 [details] [diff] [review]
Stab #2

>+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
>+                              getter_AddRefs(appUserChromeDir));

Indent to align inside the paren (I assume you added the "nsresult" to a copied line, which is why it's out of whack).

>+  // Add our chrome manifest and CSS file before returning, so that files are
>+  // in place before registering chrome.

I think this is enough comment here, and the discussion in this bug can go into the gory details if someone wants to trace it back.

>+  NS_ADDREF(*outFolder = appUserChromeDirLocalFile);

If I understand question 9, you are basically asking if this line is correct? If so, yes, definitely.


It still feels a bit funny to be making files here, but given the method you copied it sounds like that's how this is structured in the Gecko side too, so maybe it's just me :) Either way, the code looks good, this layout seems cleaner to me, and I can see how this would be really helpful for power users; sr=me. Thanks for putting up with my ignorant questions about other approaches!
Attachment #543351 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
(In reply to comment #20)
> >+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR,
> >+                              getter_AddRefs(appUserChromeDir));
> 
> Indent to align inside the paren (I assume you added the "nsresult" to a
> copied line, which is why it's out of whack).

Guilty as charged :-)

> >+  NS_ADDREF(*outFolder = appUserChromeDirLocalFile);
> 
> If I understand question 9, you are basically asking if this line is
> correct? If so, yes, definitely.

And the following |return NS_OK|, but yes, that.

> It still feels a bit funny to be making files here, but given the method you
> copied it sounds like that's how this is structured in the Gecko side too,

Agreed.  I think if we were a XUL app that did other stuff with chrome and profile files we'd have a better place to do it, but since we're not…

Thanks for all the help here (and from you, too, philippe)!

Landed as http://hg.mozilla.org/camino/rev/a4313852d465 with comments addressed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Ts impact from this appears to be within the range of noise :-)
You need to log in before you can comment on or make changes to this bug.