565610 - Can not overlay about: urls

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Brian King [:kinger]]

In the Add-on Compatibility Reporter, we are using the simple chrome.manifest declaration to overlay the Add-ons Manager:

overlay   chrome://mozapps/content/extensions/extensions.xul chrome://acr/content/view/extensionsOverlay.xul

The overlay is not loaded in the new add-ons manager XUL page. I have verified this by using DOM Inspector, plus an alert not firing in a function called via a load event listener.

If I load the Add-ons Manager via the following alternative route, the overlay is applied and the alert shows:

javascript:open("chrome://mozapps/content/extensions/extensions.xul", "_blank", "chrome=no");

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a5pre) Gecko/20100511 Minefield/3.7a5pre (.NET CLR 3.5.30729)

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I had blindly assumed that would work. And if this is the case for all chrome:// URLs loaded in a tab, then its going to be quite an issue as more things get moved from being windows/dialogs to being loaded in tabs (eg, preferences, download manager, places library, etc).

Neil: Any ideas? Or ideas on anyone else I should ask? I'm really hoping this is something that can be fixed.

Comment 2

[Neil Deakin]

A test on a simple overlay works fine. So may be something specific to the extensions.

Is extensions.xul loaded in the normal way? That it applies when calling open, as described in the first comment, implies that the manner in which the file is loaded and displayed in involved.

Comment 3

[Neil Deakin]

Ah, is the extension window actually being loaded as about:extensions or somesuch? An overlay on chrome://mozapps/content/extensions/extensions.xul won't work.

Overlays also only work for chrome urls.

Comment 4

[Erik Vold]

"is the extension window actually being loaded as about:extensions or
somesuch?"

Yes about:addons

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yes, its normally loaded as about:addons. But Brian said to me that he also tried loading the chrome:// URL directly with no success. Brian - can you confirm?

Would there be any possibility of getting overlays working for about: URLs? Or is there something the about: protocol handler can do to make them work?

AFAIK, the plan is to introduce more chrome pages loaded in tabs as about: URLs. Not being able to load overlays on those pages would be a deal-breaker; and we'd have to consider using chrome:// URLs instead, which really isn't wanted.

Comment 6

[Brian King [:kinger]]

(In reply to comment #5)
> Yes, its normally loaded as about:addons. But Brian said to me that he also
> tried loading the chrome:// URL directly with no success. Brian - can you
> confirm?

Sorry, I was mistaken. I just tested via chrome://mozapps/content/extensions/extensions.xul and the overlay *is* loaded.

> Would there be any possibility of getting overlays working for about: URLs? Or
> is there something the about: protocol handler can do to make them work?
> 
> AFAIK, the plan is to introduce more chrome pages loaded in tabs as about:
> URLs. Not being able to load overlays on those pages would be a deal-breaker;
> and we'd have to consider using chrome:// URLs instead, which really isn't
> wanted.

Yes, there are many add-ons out there that overlay the Add-ons Manager that will be broken by this.

Comment 7

[Anthony Lieuallen]

Just weighing in to say that Greasemonkey is very interested in listing User Scripts in the standard "Add Ons" dialog, wherever it may be, as we view User Scripts as just another kind of add on for the browser, and believe it's simplest/best to present them that way to users.  We would desperately appreciate if the Firefox developers could reach out to us as this change progresses, to make sure that this is both possible and straightforward (i.e. without fragile techniques).

Our developer list is greasemonkey-dev @googlegroups.com .  Thanks in advance.

Comment 8

[Dave Townsend [:mossop]]

The last time we came across this we closed it as basically intentional. I wonder how hard it would be to implement though.

Comment 9

[Dave Townsend [:mossop]]

Allowing overlays on about: urls turns out to be very easy, at least on the
surface. What I don't know is if there was a specific reason it is disabled
right now. Do you know Boris?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Where is it disabled?

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Perhaps easiest answered with this WIP patch I have that enables overlays for about: uris. I still need to verify what that means for unprivileged about: uris.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, why not ask Benjamin, who added those checks?  They were added in bug 229285.

Comment 13

[Benjamin Smedberg]

IIRC it had something to do with fastload of overlays and that the fastload cache was correct if you loaded a page via chrome: first and then via about:, but not the other way around. I don't have a strong preference as long as it works consistently.

Comment 14

[Brian King [:kinger]]

So we have a patch here, and what seems like agreement that we can do this.

Can we check in?

Comment 15

[Boris Zbarsky [:bzbarsky]]

We have agreement?  What we have is a note that doing this in the past broke things and that if we want to do it now we need to make sure said things aren't broken first.  Ideally in automated test form, of course.  See comment 13.

Comment 16

[Dave Townsend [:mossop]]

Yeah I need to work out how to verify and if possible test the problem mentioned in comment 13.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 445373 [details] [diff] [review]
patch rev 1

I think this patch is correct, at least as far as my understanding goes here. From bsmedbergs concerns I have verified that it works regardless of whether you load the about: page or chrome: page first. I couldn't figure out exactly whether the documents were going into the XUL cache correctly, I'm not sure how to do that. I also don't know how we can automatically test this easily since we'd have to tell the chrome registry about new overlays somehow. I can easily provide an add-on that overlays about:addons to prove it works in a litmus test though if we have nothing else.

This should cover overlay and style overlays.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> From bsmedbergs concerns I have verified that it works regardless of whether
> you load the about: page or chrome: page first.

Hmm.  So what I seem to recall is that scripts in overlays are compiled with the principal of the document being overlayed and then cached in the xul proto cache.  Which means that if you overlay an about: page which is not system-principal, it will "break" the scripts in that overlay if it's applied to any system-principal document, right?  And due to fastload, that will persist across restarts, even....

I guess the IsChromeURI guard in nsXULDocument::OnStreamComplete is staying, though, right?

Should we add conditions in nsXULDocument::LoadOverlayInternal to not stick overlays being loaded into the proto cache if |this| is not chrome?  It would be slower for about: uses which use overlays, but safer, I think.

Comment 19

[Brian King [:kinger]]

Attachment: ACR dev XPI

Dave, in comment 17 you were looking for an add-on to test. This attachment is a dev version of the Add-ons Compatibility Reporter.

There is only a content overlay, not a style one:

overlay   chrome://mozapps/content/extensions/extensions.xul chrome://acr/content/view/extensionsOverlay.xul

What you should see is a Compatibility button in the listings in the add-ons window.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch rev 1

(In reply to comment #18)
> Should we add conditions in nsXULDocument::LoadOverlayInternal to not stick
> overlays being loaded into the proto cache if |this| is not chrome?  It would
> be slower for about: uses which use overlays, but safer, I think.

Added that. I've also made it so we ignore overlays in the cache when the main document is not chrome to avoid the case where the main document does not have the system principal but the cached overlay would, which I presume would also be bad.

It might be nice to make it so we really check if the main document has the system principal or not rather than just if it is chrome at some point but I think this works for now. I've verified with some logging that we don't read/write the cache when loading about: urls with overlays but still do for chrome:.

Comment 21

[Dave Townsend [:mossop]]

Per the planning meeting we want the add-on compatibility reporter available and working in time for beta1 and this bug blocks that so it is a beta blocker.

Comment 22

[Brian King [:kinger]]

NEW ACR test version compatible with 3.7a6pre:
https://bugzilla.mozilla.org/attachment.cgi?id=450640

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

So does that new version work with the trunk? Or is it still blocked on this bug?

bz: any chance of a review early next week?

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #23)
> So does that new version work with the trunk? Or is it still blocked on this
> bug?

ACR is still blocked by this.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> bz: any chance of a review early next week?

Yes.  Maybe even tonight, if I really need a break from this e10s merge.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 449284 [details] [diff] [review]
patch rev 1

>+++ b/content/xul/document/src/nsXULDocument.cpp
>+    // why is this check a member function of nsXULDocument? -gagan

It's not, and gagan's not involved.  Just drop that line, please.

>+    // the cached document will do, see bug 565610.

s/will do/will/

r=bzbarsky

Comment 27

[Dave Townsend [:mossop]]

Thanks, landed: http://hg.mozilla.org/mozilla-central/rev/5f3f36fe8d76

Comment 28

[David McNamara [:mackers]]

Confirmed WORKING with ACR with following entry in chrome.manifest:

overlay about:addons chrome://acr/content/view/extensionsOverlay.xul

r69115

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Tested with latest developer release of ACR and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre on all platforms. The Add-ons Manager can be overlayed now. Marking as verified fixed.