Last Comment Bug 549539 - Send a (chrome|content)-document-global-created notification after a new window is created, but before any script inside it has run
: Send a (chrome|content)-document-global-created notification after a new wind...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a3
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
: 523896 526010 (view as bug list)
Depends on: 642457 542428 549452 582176 608669
Blocks: 462483 342715 546848 552140 567357
  Show dependency treegraph
 
Reported: 2010-03-01 20:49 PST by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2011-03-17 08:13 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.7+
.7-fixed
wontfix


Attachments
Patch to fix (3.34 KB, patch)
2010-03-01 20:49 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
bzbarsky: superreview+
christian: approval1.9.2.4-
Details | Diff | Splinter Review
Example JS component (1.67 KB, text/plain)
2010-03-01 21:24 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details
192 branch patch (2.83 KB, patch)
2010-06-11 15:34 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
christian: approval1.9.2.7+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-01 20:49:17 PST
Created attachment 429659 [details] [diff] [review]
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 2010-03-01 20:59:14 PST
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 2010-03-01 21:00:02 PST
cc'ing security folks. We'll definitely want to do a security review on this.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2010-03-01 21:20:16 PST
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 2010-03-01 21:24:24 PST
Created attachment 429661 [details]
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 2010-03-01 21:30:27 PST
(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 Ted Mielczarek [:ted.mielczarek] 2010-03-02 04:26:15 PST
(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 [:bz] (still a bit busy) 2010-03-02 09:32:28 PST
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 2010-03-02 09:39:41 PST
(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) 2010-03-02 17:51:56 PST
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 10 Ted Mielczarek [:ted.mielczarek] 2010-03-03 07:20:17 PST
*** Bug 523896 has been marked as a duplicate of this bug. ***
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 22:25:04 PST
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 2010-03-10 13:52:50 PST
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 2010-03-13 12:34:11 PST
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 2010-03-15 14:13:13 PDT
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 2010-03-15 14:31:30 PDT
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 2010-03-22 14:05:21 PDT
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 2010-05-26 11:42:39 PDT
This would also be very useful in 3.6.x for Jetpack Page mods (bug 546739).
Comment 18 christian 2010-06-01 16:34:04 PDT
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 19 Myk Melez [:myk] [@mykmelez] 2010-06-10 16:45:17 PDT
*** Bug 526010 has been marked as a duplicate of this bug. ***
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-06-11 15:34:07 PDT
Created attachment 450759 [details] [diff] [review]
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 2010-06-11 16:06:40 PDT
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] 2010-06-13 17:36:46 PDT
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 christian 2010-06-18 10:43:00 PDT
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 2010-06-24 21:06:46 PDT
Checked in on 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a48a07f71983
Comment 25 Eric Shepherd [:sheppy] 2010-08-31 06:34:16 PDT
I've added these to the list of window notifications here:

https://developer.mozilla.org/en/Observer_Notifications#Windows
Comment 26 Eric Shepherd [:sheppy] 2010-08-31 06:36:52 PDT
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 [:bz] (still a bit busy) 2010-08-31 06:45:48 PDT
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] 2010-08-31 06:55:30 PDT
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 2011-03-13 21:34:08 PDT
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 2011-03-14 00:17:39 PDT
(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 2011-03-14 08:48:32 PDT
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 [:bz] (still a bit busy) 2011-03-14 08:58:07 PDT
> 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 2011-03-14 08:59:36 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.