1034999 - XUL overlays should ignore hash portion of urls for about:preferences

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Carlos [nohamelin]]

With the landing of bug 754304, now is possible to open specific views of the in-content preferences page with urls as:

* about:preferences#general
* about:preferences#content
* ...
* about:preferences#

I see that chrome overlays defined for about:preferences aren't being applied(?) if these new urls are used for opening the page. It affects to add-ons: I noted it with one of mine (Simple Locale Switcher); I remember having seen a few other extensions doing it too (tabs-related add-ons).

xul overlays should ignore the hash portion of the url for these cases.

Comment 1

[:Gijs (he/him)]

Looking at this for only a few minutes, at a minimum, the code at http://hg.mozilla.org/mozilla-central/annotate/1dc6b294800d/content/xul/document/src/XULDocument.cpp#l2624 will need to strip hashes in order for this to "just work".

However, this generally makes me suspect that the whole xul overlay system doesn't deal well with hashes, and that will also mean that the fastload / xul cache system will store different prototype documents for loading chrome://foo/bar/baz.xul#1 and chrome://foo/bar/baz.xul#2, which is probably wrong and would need to be corrected here: http://hg.mozilla.org/mozilla-central/annotate/1dc6b294800d/content/xul/document/src/XULDocument.cpp#l395 or in the xul prototype code so that the base URI strips out hashes...

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

Any idea what's a good component for this?

Comment 3

[:Gijs (he/him)]

(not super sure this needs to block shipping the in-content prefs, but it would be a loss for add-on compat/reliability if we didn't fix this in time, so I would lean to 'yes' - then again, not sure what magnitude the other blockers are)

Comment 4

[:Gijs (he/him)]

Stealing. Also, I realized we can't really automated-test this because bootstrapped manifests aren't allowed overlay or style registrations. :-(

Comment 5

[:Gijs (he/him)]

Attachment: Test add-on

Here's a silly test add-on. It'll add an "Extra" item at the top of the categories list, and make the right-hand pane look green. It'll otherwise completely break about:preferences (because of the extra category), but that isn't the point.

This should do the above on both about:preferences and about:preferences#foo (and for any other #somehash)

Comment 6

[:Gijs (he/him)]

Attachment: fix XUL cache and overlay system to ignore refs/hashes,

I think this is sufficient. I've not touched the prototype's mURI, which AFAICT is meant to stay the same as the documentURI...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8489410 [details] [diff] [review]
fix XUL cache and overlay system to ignore refs/hashes,

># HG changeset patch
># User Gijs Kruitbosch <[e-mail]>
># Date 1410795261 -3600
>#      Mon Sep 15 16:34:21 2014 +0100
># Node ID 73d6229cab1be1bcc0647e90c61a928a35f9e543
># Parent  00d51a916d6db23cf67ae19a6131440853bf9b87
>Bug 1034999 - fix XUL cache and overlay system to ignore refs/hashes, r?smaug
>
>diff --git a/content/xul/document/src/XULDocument.cpp b/content/xul/document/src/XULDocument.cpp
>--- a/content/xul/document/src/XULDocument.cpp
>+++ b/content/xul/document/src/XULDocument.cpp
>@@ -521,23 +521,28 @@ XULDocument::EndLoad()
>     // the cache earlier in XULDocument::StartDocumentLoad.
>     if (useXULCache && mIsWritingFastLoad && isChrome &&
>         mMasterPrototype != mCurrentPrototype) {
>         nsXULPrototypeCache::GetInstance()->WritePrototype(mCurrentPrototype);
>     }
> 
>     if (IsOverlayAllowed(uri)) {
>         nsCOMPtr<nsIXULOverlayProvider> reg =
>             mozilla::services::GetXULOverlayProviderService();
> 
>+        nsAutoCString specWithoutRef;
>+        uri->GetSpecIgnoringRef(specWithoutRef);
>+        nsCOMPtr<nsIURI> uriWithoutRef;
>+        NS_NewURI(getter_AddRefs(uriWithoutRef), specWithoutRef, nullptr);
>+
>         if (reg) {
>             nsCOMPtr<nsISimpleEnumerator> overlays;
>-            rv = reg->GetStyleOverlays(uri, getter_AddRefs(overlays));
>+            rv = reg->GetStyleOverlays(uriWithoutRef, getter_AddRefs(overlays));
Wouldn't it make more sense, and the API less error prone if GetStyleOverlays
would deal with refs.
Also, doesn't cloneIgnoringRef work for you?

Comment 8

[:Gijs (he/him)]

Attachment: Test add-on

s/red/green/

Comment 9

[:Gijs (he/him)]

Attachment: fix XUL cache and overlay system to ignore refs/hashes,

Good point on both counts - I missed cloneIgnoringRef, for some reason...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8489510 [details] [diff] [review]
fix XUL cache and overlay system to ignore refs/hashes,

> nsChromeRegistryChrome::GetStyleOverlays(nsIURI *aChromeURL,
>                                          nsISimpleEnumerator **aResult)
> {
>-  const nsCOMArray<nsIURI>* parray = mStyleHash.GetArray(aChromeURL);
>+  nsCOMPtr<nsIURI> chromeURLWithoutHash;
>+  aChromeURL->CloneIgnoringRef(getter_AddRefs(chromeURLWithoutHash));
Could you do
if (aChromeURL) {
  aChromeURL->CloneIgnoringRef(getter_AddRefs(chromeURLWithoutHash));
}
That way the method doesn't start to crash or behave differently in case someone, 
like a js-based addon, calls it with null.

> nsChromeRegistryChrome::GetXULOverlays(nsIURI *aChromeURL,
>                                        nsISimpleEnumerator **aResult)
> {
>-  const nsCOMArray<nsIURI>* parray = mOverlayHash.GetArray(aChromeURL);
>+  nsCOMPtr<nsIURI> chromeURLWithoutHash;
>+  aChromeURL->CloneIgnoringRef(getter_AddRefs(chromeURLWithoutHash));
ditto

Do we need to care about mStyleHash/mOverlayHash containing URLs which actually do have refs?
And same applies to skins too.
Perhaps http://mxr.mozilla.org/mozilla-central/source/chrome/nsChromeRegistryChrome.cpp?rev=7f5a7dc73420#724 should drop refs?



> nsXULPrototypeCache::GetPrototype(nsIURI* aURI)
> {
>-    nsXULPrototypeDocument* protoDoc = mPrototypeTable.GetWeak(aURI);
>+    nsCOMPtr<nsIURI> uriWithoutRef;
>+    aURI->CloneIgnoringRef(getter_AddRefs(uriWithoutRef));
I'd add similar null check here as well.
At least I couldn't easily see what guarantees aURI is always non-null here.



>-    nsCOMPtr<nsIURI> uri = aDocument->GetURI();
>+    nsCOMPtr<nsIURI> uri;
>+    aDocument->GetURI()->CloneIgnoringRef(getter_AddRefs(uri));
GetURI() hints strongly (Get* prefix) that it may return null in some cases.
So null check here too.


r+ to this, but I think nsChromeRegistryChrome shouldn't store urls with refs.

Comment 11

[:Gijs (he/him)]

Attachment: fix XUL cache and overlay system to ignore refs/hashes,

OK. I actually think that the proto cache cases should always be non-null, and there are various bits of code that would already fail if this were not the case (e.g. BeginCaching called from GetPrototype derefs aURI without nullchecking), but I've added the nullchecks you asked for, returning early in some cases. Please let me know if this is OK. :-)

Comment 12

[Benjamin Smedberg]

See related bug 305393 where we different search strings must be treated separately. But I think different hashes is probably ok to unify.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8490011 [details] [diff] [review]
fix XUL cache and overlay system to ignore refs/hashes,

Review of attachment 8490011 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have any way to test this?

Comment 14

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #13)
> Comment on attachment 8490011 [details] [diff] [review]
> fix XUL cache and overlay system to ignore refs/hashes,
> 
> Review of attachment 8490011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have any way to test this?

Not that I know of, because overlay and style registrations are only allowed in non-restartless add-ons, so it'd require a restart to test, AIUI? Maybe I'm wrong and there's some way to test this with a gtest of sorts, with some compiled code that calls into the non-exported relevant methods? I would be skeptical about that being worth it.

remote:   https://hg.mozilla.org/integration/fx-team/rev/28c446b22a15

Comment 15

[Benjamin Smedberg]

It should be possible to test this by registering a manifest using nsIComponentRegistrar.autoRegister pointing to a test-only manifest.

Comment 16

[:Gijs (he/him)]

I'll have a look at using autoRegister. Thanks!

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28c446b22a15

Comment 18

[:Gijs (he/him)]

Splitting off the test portion into bug 1071714 for backlog admin purposes.

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Assigning to Camelia for verification since she has the time to take it.

Comment 20

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 35.0a1 (buildID: 20140926030202).

Comment 21

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as Verified since there is no Target Milestone set.