549539 - Send a (chrome|content)-document-global-created notification after a new window is created, but before any script inside it has run

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

We've for a long time wanted to get rid of enablePrivilege. However we have two blocking factors in this:

1. We want to allow companies to create an extension that exposes functionality
   to intranet pages and thus replace the need for enablePrivilege.
2. We need to do privileged things in our internal automated tests, especially
   mochitest

Patch coming up that adds a notification which extensions can listen to and use to inject API where needed. This notification is sent whenever we set up a new inner window. Set up == create new, or reuse existing.

One big question is for which windows to send out this notification. We probably don't want random intranet extensions mucking around with our internal chrome windows just because they're adding APIs to intranet pages. Though of course, if they do proper origin checks, that should be happening.

At the same time, I'd imaging that a notification for when new inner windows are set up can be useful for many other things, in which case chrome windows will likely be interesting too (think chromebug).

For now I went with two notifications, chrome-document-window-created and content-document-window-created. Would be nice to drop the "document" part, but then it sounds like the difference is between nsGlobalWindow and nsChromeGlobalWindow, which is not the case. Naming suggestions welcome.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

An alternative approach, to avoid having people forget to check the origin before adding sensitive APIs to a window, is to create categories like:

window-created-http://example.com

or some such. So we'd look up the category "window-created-" + origin, and call all objects in that category. Though that makes it hard to do things like add objects to *.intranet.company.com

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

cc'ing security folks. We'll definitely want to do a security review on this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

we _could_ do lookups for various versions of the origin with more and more subdomains stripped....  But that could get a bit slow, perhaps.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Example JS component

This is an example of what a component taking advantage of this new API could look like.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #3)
> we _could_ do lookups for various versions of the origin with more and more
> subdomains stripped....  But that could get a bit slow, perhaps.

Yeah, I was thinking about that too. I'd be concerned about both perf and complexity. Plus there's a risk that someone could add stuff to http://company.com, not realizing that that would also get http://external.company.com. Though we could get into the business of wildcards and stuff, but that gets even more complex.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to comment #1)
> An alternative approach, to avoid having people forget to check the origin
> before adding sensitive APIs to a window, is to create categories like:
> 
> window-created-http://example.com
> 
> or some such. So we'd look up the category "window-created-" + origin, and call
> all objects in that category. Though that makes it hard to do things like add
> objects to *.intranet.company.com

For Mochitest, we have a hardcoded list of domains we support, so this approach would be fine. It also sounds like it's the least likely to be accidentally misused.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So a few questions....

1)  This needs to be applied on top of the patch in bug 549452, right?
2)  Does this fix bug 342715?  Sure looks like it does.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #7)
> 1)  This needs to be applied on top of the patch in bug 549452, right?

Indeed, hence the dependency.

> 2)  Does this fix bug 342715?  Sure looks like it does.

At least it's *one* fix for it. I'll comment over in that bug.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 429659 [details] [diff] [review]
Patch to fix

I talked to jst through the various failure modes of firing of the notification from the end of SetNewDocument and we decided that it's safe. So this looks good to me!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 429659 [details] [diff] [review]
Patch to fix

How about trusted-document-created and untrusted-document-created (or "chrome" and "content" instead of trusted/untrusted) without even mentioning windows?  The docs for the notification could say that the subject is the document's window.

Or maybe global-for-chrome-document-created and global-for-content-document-created?  Since inner windows aren't really "windows" so much as JS global objects...

sr=bzbarsky, in any case.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll go with chrome-document-global-created and content-document-global-created

Waiting to land this until the two dependencies are fixed, so might want to hold off publishing docs yet.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed!! Thanks for the reviews!

http://hg.mozilla.org/mozilla-central/rev/a9b980fcb73a

This shouldn't be hard to backport to 1.9.2 if we want to do that btw. I'll let other people nominate if they want it.

Comment 14

[John J. Barton]

If this can be included in a version of FF 3.6, we can use it in Firebug 1.6. Otherwise we have to wait for Firebug 1.7 (FF 3.8). 

BTW we expect this event to go a long ways to clarify puzzling Firebug code. Thanks!

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 429659 [details] [diff] [review]
Patch to fix

FWIW, I don't think this bug is a blocker, but if Firebug could make use of it then I'll request approval.

Comment 16

[Nickolay_Ponomarev]

Updated the summary to mention the actual changes made here.

Also cross-referencing some discussions about this:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/ac641787f5505920?pli=1

Discussion of exposing an API using this notification:
http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/6ac9261d44b89948/d4ac2f3ee7338156?tvc=1#d4ac2f3ee7338156

Comment 17

[Nickolay_Ponomarev]

This would also be very useful in 3.6.x for Jetpack Page mods (bug 546739).

Comment 18

[LegNeato]

A couple of things...

1. We'll need a roll-up with the fix for bug 549452 if this patch needs to be applied on top

2. I'd like to know a little more about the risks in taking this (and bug 549452). I understand the rewards for pagemods and firebug but the risks aren't really spelled out. Does this have the potential to break current extensions? Or is this an additional event on top of what is already there? Could there be security implications to exposing this event?

Thanks!

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 192 branch patch

This should do it. Also includes porting bug 567357 to the branch as they go hand in hand.

Turns out it was quite easy to port to branch even though bug 549452 hasn't been (and shouldn't be) ported to there.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To answer the questions in comment 18:

There is virtually no risk that this will break existing extensions. All the patch does is add new notifications so existing stuff should not be affected.

As for security risks, I'll let Blake answer, it might be a problem that we don't have bug 542428 on the branch yet.

mrbkap, what do you think?

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 450759 [details] [diff] [review]
192 branch patch

I tested this branch patch against a set of unit tests for a Jetpack module that uses this functionality.  On Firefox 3.6.3 release, the unit tests fail because they time out waiting for the notification.  On a build of the latest 1.9.2 branch code with this patch applied, the unit tests pass, just as they do on a trunk nightly build.  So the patch behaves as expected.

Comment 23

[LegNeato]

Comment on attachment 450759 [details] [diff] [review]
192 branch patch

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a48a07f71983

Comment 25

[Eric Shepherd [:sheppy]]

I've added these to the list of window notifications here:

https://developer.mozilla.org/en/Observer_Notifications#Windows

Comment 26

[Eric Shepherd [:sheppy]]

OK, I'm confused by this. The 1.9.2 patch calls these *-document-global-created, but the 2.0 patch calls them *-document-window-created. What's up with that?

Comment 27

[Boris Zbarsky [:bzbarsky]]

The attached patch differs from what was landed for 2.0.  See comment 12 for the relevant part of the discussion, and http://hg.mozilla.org/mozilla-central/rev/a9b980fcb73a for what was checked in.

Comment 28

[Eric Shepherd [:sheppy]]

I've moved these notifications to a new Documents section on the Observer Notifications page, and have corrected their names and added the fact that their subjects are an nsIDOMWindow:

https://developer.mozilla.org/en/Observer_Notifications#Documents

Comment 29

[John J. Barton]

I started to use this new feature with high hopes, but so far it's not as much fun as I expected ;-).

Contrary to the documentation, 
  |data| seems to be |null| for chrome-document-global-created always.
  |data| seems to be |null| for content-document-global-created sometimes and the rest of the time it's the scheme://host, not the uri of the content.

What's more troublesome are events for data: null and location blank (not "about:blank", just blank). Plus events for data: null and location "about:blank", which I guess is the about:blank window created before the real window, except in those cases where the user really opened about:blank.

Maybe that new window id thing will help sort this out....

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #29)
> I started to use this new feature with high hopes, but so far it's not as much
> fun as I expected ;-).
> 
> Contrary to the documentation, 
>   |data| seems to be |null| for chrome-document-global-created always.
>   |data| seems to be |null| for content-document-global-created sometimes and
> the rest of the time it's the scheme://host, not the uri of the content.

|data| is the origin (for use in security checks), so it's intended just to be scheme://. And obviously for chrome the origin is always the system so it's intentionally left blank.

However it should never be null for content-document-global-created so please file a separate bug for that.

> What's more troublesome are events for data: null and location blank (not
> "about:blank", just blank). Plus events for data: null and location
> "about:blank", which I guess is the about:blank window created before the real
> window, except in those cases where the user really opened about:blank.
> 
> Maybe that new window id thing will help sort this out....

I don't follow what you are saying here. If you think there is a bug please file a bug and attach a testcase.

Comment 31

[John J. Barton]

I attempted to correct the documentation page, please check it.

What is the significance of an nsIDOMWindow with location blank? Should the debugger ignore these because there is nothing a developer can ever do with it? Or should I build a representation for these? If so how should I help the developer understand the purpose of this object?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> What is the significance of an nsIDOMWindow with location blank?

Blank in what sense?  Empty string?  That shouldn't ever happen...

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A testcase would be helpful.

However, in some cases we reuse inner windows. I.e. they are created with a blank location before we know what the final location will be. Once we know what the final location is we reuse the inner window to save on performance and fill it with the new document.

Since we always send out a content-document-global-created notification when a window is created, I didn't want to re-notify once the window is "filled" with a new document. Would be nice if we in that case only sent out the notification once filling the window with a real document though I'm not sure if this is always possible.

Feel free to file a separate bug on this.