Closed Bug 820953 Opened 9 years ago Closed 7 years ago

Expose DOM window to the add-on

Categories

(Add-on SDK Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: irakli)

References

Details

Attachments

(8 files)

At the moment there are bunch of work one needs to do to expose
all the basic DOM APIs like  Bug 806834, Bug 786644 and more...

It would be a lot easier for add-on devs to have access to a
hidden-window that would provide all the DOM APIs out of the box! Also they
won't have to learn anything to consume stuff they already know!!
Gabor, I'd like to get some feedback from you on this subject. Mainly what URL should we load into such hidden window to be able to use all the DOM APIs without giving away too much capabilities.
Assignee: nobody → rFobic
I CCed Boris, I really don't know enough to make any call here. I'm a bit scared of the hidden window approach in general, and platform developers in general had quite negative feeling about this so far. But maybe there is a sane way to do this, I definitely see the pros of it. I just don't know how would hidden windows work out on different platforms and what kind of consequences we should face (performance, more difficult addon review from security perspective maybe, life time management of the window, etc) and what kind of option gecko has to offer for us first of all. So what we're talking about here is for _some_ jetpack modules we could use a chrome window (without any visual representation) instead of a chrome sandbox.
One suggestion ( and I may be wildly wrong about this ) - it was mentioned the other day that having a custom uri scheme for addons would give us more control over what capabilities that uri scheme provided to the document. This was in reference to iframes though - would it be of any help here?
Well, comment 1 and 2 sound like this would be _a_ hidden window, not _the_ magic hidden window.

That said, any url you load is likely to be problematic if the window type is not set correctly, because some security checks depend on window type (editor vs mailnews vs browser) last I checked....  If you don't care about that part, loading about:blank or "data:text/plain," seems like the obvious thing.

Note bug 565388 by the way.
Ok so I have tried couple of things to get things moving. My first attempt was to use of mozbrowser type iframes (see Bug 822909) injected into a hidden window, which unfortunately failed since swapping frameLoaders on such frames does not seems to be implemented.

My another my attempt was to create a top level hidden window for an add-on itself:
https://gist.github.com/4347249

Code seems to work, only strange thing is that window get's closed if reference to xulWindow is no kept, donno if that's intended or if it's a bug.


> That said, any url you load is likely to be problematic if the window type is not > set correctly, because some security checks depend on window type (editor vs
> mailnews vs browser) last I checked....  If you don't care about that part,
> loading about:blank or "data:text/plain," seems like the obvious thing.

Ideally we'll be loading a add-on's own page there itself that should have a content principles maybe with some extra capabilities for working with content from multiple domains.

Are any of the options above reasonable ?


> Note bug 565388 by the way.

That's would be really interesting, would there be any way to display content document from hidden docShell into let's say in door hanger panels ? At the moment
we create such panels with iframe in them and have another iframe in hidden window and we swap frame loaders between these two iframes when panel is shown / hidden.
That way state of the content loaded in the panel is preserved across the windows and show / hides. Would anything like this be possible with invisible docshells ?
Depends on: 822909
> That's would be really interesting, would there be any way to display content document
> from hidden docShell into let's say in door hanger panels ?

Probably only using things like SwapFrameLoaders, but using those on subframes of the document in your hidden window should work, yes.  I really need to try to get bug 565388 done, but it might not be until the new year at this point.  :(
I made prototype implementation using top level hidden windows that is created per add-on. Once Bug 565388 is fixed we can migrate to invisable docShells.
Attachment #694584 - Flags: review?(evold)
Well for one thing I'd like to see some tests that the XHR object provided by the hidden DOM is useless.  Also I'm not sure what we should do when the users tries to open a frame, or include scripts in their background page.. should we just leave that to the AMO reviewers?  That seems to break our goal of knowing what permissions an add-on is requesting to use.

fwiw Google Chrome is now offering an alternative to background pages because they are memory hogs: http://blog.chromium.org/2012/06/put-your-extensions-on-diet-with-event.html

So I'd really not like bloating all existing SDK based add-ons, non of which need the background window, maybe an opt-out would be fine.
>
> fwiw Google Chrome is now offering an alternative to background pages
> because they are memory hogs:
> http://blog.chromium.org/2012/06/put-your-extensions-on-diet-with-event.html
> 
> So I'd really not like bloating all existing SDK based add-ons, non of which
> need the background window, maybe an opt-out would be fine.
>

My understanding of Bug 565388 is that invisible docShells will be cheaper then regular content tabs so I don't think we should be concerned too much about memory, also note that most of our APIs make use of hidden frames anyway so only thing changing is that we'll actually expose it to users.
(In reply to Boris Zbarsky (:bz) from comment #4)
> Well, comment 1 and 2 sound like this would be _a_ hidden window, not _the_
> magic hidden window.
> 
> That said, any url you load is likely to be problematic if the window type
> is not set correctly, because some security checks depend on window type
> (editor vs mailnews vs browser) last I checked....  If you don't care about
> that part, loading about:blank or "data:text/plain," seems like the obvious
> thing.
> 
> Note bug 565388 by the way.

Back to this one, we'll need to load html pages bundled with add-on into it but
they will have to be restricted in capabilities, such that they'll have to work
with specific set of domains. I believe we should create invisable docShells load
"data:text/plain," into it. And then create a content type `iframe` in the document where add-on's html page can be loaded. Although it's unclear to me how can we have
document that can interact with multiple domains but, have less capabilities than chrome. Only solution I suspect may work is by defining our custom protocol handler where newChannel would return channels with channel.owner set to special nsIPrincipal that would allow interaction with a list of specific domains.

Boris: Would that be a right way to go about it ?
> My understanding of Bug 565388 is that invisible docShells will be cheaper

I believe that understanding is wrong.  If you want all the functionality of a regular content tab (scripting, layout, etc), then you pay the full memory cost of a content tab...

> Although it's unclear to me how can we have document that can interact with multiple
> domains but, have less capabilities than chrome. 

I believe the expanded principal stuff the b2g folks did might give you something like this...  But yes, in terms of getting that principal on a document you end up having to do something that stamps the .owner on channels or changes the behavior of getChannelPrincipal.

A custom protocol handler might be simplest, yes.
Comment on attachment 694584 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/698

Hey Irakli,

I'm going to r- this mainly because it needs some tests.

Besides that I think it won't be very safe, and may encourage bad practices, and afaict this patch loads a hidden window for every add-on regardless if the "addon/window" module is used or not.

I'd like to have Mossop review this too.
Attachment #694584 - Flags: review?(evold) → review-
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(dtownsend+bugmail)
Comment on attachment 694584 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/698

Waiting on feedback in the pull request.
Attachment #694584 - Flags: review?(dtownsend+bugmail) → review-
Attachment #694584 - Flags: review- → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/42cede06862471f3c05cb937cb3d2200b4d3218e
Merge pull request #698 from Gozala/bug/addon-window@820953

Bug 820953 - Expose DOM window to the add-on r=@Mossop
## Overview

Ok so here is a little overview of what we do:

1. At app (or addon) start-up top level hidden window is created using appShellService.createTopLevelWindow:

https://github.com/Gozala/addon-sdk/blob/bug/addon-window%40820953/lib/sdk/window/utils.js#L143

No `opitions` are passed so all args fallback to defaults.

2. Then created top level window is unregistered, using `appShellService.unregisterTopLevelWindow`:

https://github.com/Gozala/addon-sdk/blob/bug/addon-window%40820953/lib/sdk/window/utils.js#L99


3. And finally document "data:text/html;charset=utf-8,<html/>" is loaded into
that window.
https://github.com/Gozala/addon-sdk/blob/bug/addon-window%40820953/lib/sdk/addon/window.js#L29


## Problem

On Linux (ubuntu) & windows closing last window does not exits the
firefox process, presumably because our top level hidden window prevents that.
On OSX it works just fine. Expectation is that our unregistered hidden window
will be closed by an add-on when it's shutdown hook is invoked, which never
seems to happen.

Also surprisingly enough `quit-application-requested` observer service notification
is no longer fired. 

Only way I managed this thing to work is to observe for `xul-window-destroyed` notifications and close our top level hidden window once nsIWindowMediator's
enumerator contains no windows:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWindowMediator#getEnumerator%28%29

That seems really dirty and I would rather expect that our hidden window would be
just closed app exited as normal.


Eddy do you wanna look into what's going wrong on the platform side since you've being digging into this stuff before.
Flags: needinfo?(ejpbruel)
Boris could you please confirm that there is nothing wrong with the steps described in overview above & that expectation of our hidden window closing at shutdown is correct.
Flags: needinfo?(bzbarsky)
I have no idea whether there's anything wrong with the steps offhand; I don't really know the details of how the appshell service APIs work....

I do know we explicitly don't count the hidden window in the set of still-open windows that block quit on non-Mac somehow.  Presumably you want the same for these other windows of yours.

Benjamin might know more about that stuff.

But really, we should fix bug 565388 (and I'm sorry I haven't gotten to it yet) instead of creating OS-level windows...
Flags: needinfo?(bzbarsky)
As far as I know there is not particular white nor black listing of hidden window,
we just avoid them to be registered on windowwatcher and windowmediator.

We are having issues because RegisterTopLevel shouldn't ever be called for our window. And calling UnregisterTopLevelWindow isn't enough.

I think I found what exactly goes wrong.
Because RegisterTopLevelWindow ends up being called for this window, nsAppStartup receive a xul-window-registered event and call EnterLastWindowClosingSurvivalArea:
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsAppShellService.cpp#594
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#642

But as we never call close on our hidden window, *AND SHOULD NOT HAVE TO DO IT*.
(gozala: I'll repeat again, but we shouldn't not have to do anything special to make firefox quit. no listening to any quit-application-requested or xul-window-destroyed.)
The sibling xul-window-destroyed is never dispatched.
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#522

So that we can workaround this by dispatching this event.
But we really shouldn't. We should either fix bug 565388 or expose  nsAppShellService::JustCreateTopWindow (or a scriptable equivalent) in order to be able to implement nsAppShellService::CreateHiddenWindowHelper in JS.
No I'm all up for fixing bug 565388, I just don't want to wait for couple of cycles before we can use it, so I'm trying to workaround this from JS and swap with invisible docshells once they're available.
I have submitted bug 845099 regarding issue ochameau has pointed out, I think regardless of what we end up doing here, firefox should not hold prevent process
from exiting.
Comment on attachment 719295 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/815

So I ended up going the other way about implementing this, which is by using mozbrowser type html iframes (because they are html iframes it's no longer matters what is hidden frame document type is).
Attachment #719295 - Flags: review?(poirot.alex)
As said in bug 822909 comment 5, mozbrowser frames are disabled on Firefox as following pref isn't set to true: dom.mozBrowserFramesEnabled.

Some feature may still work, I think that they will still be content frames according to this code:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#691

But I'm not sure it is safe to start using them on something else than b2g.
Comment on attachment 719295 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/815

Reseting the review per previous comment. I'd like to know more about how safe it is to use mozbrowser frames in firefox given that pref story.
Attachment #719295 - Flags: review?(poirot.alex)
Comment on attachment 719295 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/815

Just need to remove `browser: true` in addon/window.
Attachment #719295 - Flags: review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/671576f91d2ac625c3f1c73f34b4d99ca5b14d7c
Merge pull request #815 from Gozala/bug/addon-window@820953

Bug 820953 - Expose DOM window to the add-on r=@ochameau
Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/671576f91d2ac625c3f1c73f34b4d99ca5b14d7c
Merge pull request #815 from Gozala/bug/addon-window@820953

https://github.com/mozilla/addon-sdk/commit/86c5fcd61c8c883f2d80d43256ba66d65b626591
Hotfix regression introduced in previous commit: Bug 820953
Attachment #749280 - Flags: review?(zer0)
Comment on attachment 749280 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/996

Looks good to me, I added minor comments, if all test are passing – on Fennec too – it's r+.
Attachment #749280 - Flags: review?(zer0) → review+
+const xulWindow = backgroundify(make())
+const docShell = xulWindow.docShell;
+
+// Get a reference to the DOM window of the given docShell and load
+// such document into that would allow us to create XUL iframes, that
+// are necessary for hidden frames etc..
+const window = docShell.contentViewer.DOMDocument.defaultView;
+window.location = "data:application/xhtml+xml;charset=utf-8,<html/>";

 Mossop repo collab 4 months ago
Is this a good url to use? Can things like websockets etc. work from data: urls?
Does this cause the window to be chrome privileged, i.e. does it have a usable Components object in it?
 Gozala repo collab 4 months ago
That's URL that bz suggested us to use for this.

I still wonder what are the plans for limiting chrome privilege in this setup. The document of this xul uri will be chrome, and I cannot change that. We could create another iframe with an expanded principal, and use its document for exposing the DOM APIs, but such an iframe would not be a xul window any longer. So the content viewer swapping the panel and hidden frame relies on would not work with that. Last time I talked to Irakli he said that separating a window for the DOM APIs and for other usages (panel, hidden frame) is not fitting in his design. Which means we have a problem here...
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0a7a8847e7ff32a8c0d7ba83445a40efa2d101d7
Merge pull request #996 from Gozala/bug/xhr@820953

Bug 820953 - Replace XHR in favour of addon/window's XHR. r=@zer0
Attachment #757562 - Flags: review?(dtownsend+bugmail)
Comment on attachment 757562 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1017

Waiting on removing the network dependency in the tests here
Attachment #757562 - Flags: review?(dtownsend+bugmail)
Attachment #757562 - Flags: review?(dtownsend+bugmail)
Comment on attachment 757562 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1017

Re-request when this is ready again.
Attachment #757562 - Flags: review?(dtownsend+bugmail)
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3f003b71587b0ab399394f73c161c50249ca0994
Merge pull request #1017 from Gozala/bug/websocket@820953

Bug 820953 - Expose web-socket API. r=@Mossop
(In reply to [github robot] from comment #38)
> Commit pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> 3f003b71587b0ab399394f73c161c50249ca0994
> Merge pull request #1017 from Gozala/bug/websocket@820953
> 
> Bug 820953 - Expose web-socket API. r=@Mossop

I had to back this out for causing test failures: https://github.com/mozilla/addon-sdk/commit/6e46b3a8900771c56be94f1a4cff76b8a8b7bf22

The tests seem to fail differently. Some examples:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24444573&tree=Jetpack
https://tbpl.mozilla.org/php/getParsedLog.php?id=24444750&tree=Jetpack
Attachment #767503 - Flags: review?(dtownsend+bugmail)
Comment on attachment 767503 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1049#attch-to-bugzilla

Didn't quite know what I was letting myself in for, this looks sane though. Check on the comments I made though.
Attachment #767503 - Flags: review?(dtownsend+bugmail) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a3aa373b4d4ae110318d1fd4a37b60f0232a715f
Merge pull request #1049 from Gozala/bug/websocket@820953

Bug 820953 - Expose web-socket API. r=@Mossop
Attachment #772942 - Flags: review?(dtownsend+bugmail)
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/8dc5b9e94d04a6e75300faf37b7037884fbdbaea
Merge pull request #1091 from Gozala/hotfix/io

Bug 820953 - Expose nodejs like fs & net modules r=@mossop
(In reply to [github robot] from comment #46)
> Commit pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> 8dc5b9e94d04a6e75300faf37b7037884fbdbaea
> Merge pull request #1091 from Gozala/hotfix/io
> 
> Bug 820953 - Expose nodejs like fs & net modules r=@mossop

And back out for causing various errors: https://github.com/mozilla/addon-sdk/commit/7ff869687f6e556e9b0f07939e25892da2a1e674
Attachment #772942 - Flags: review?(dtownsend+bugmail) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/33c93a47e2f855c7d8bce67489188be27641d711
Merge pull request #1112 from Gozala/hotfix/fs-io

Bug 820953 - Update io modules to make use of array buffers. a=@gozala
Attachment #777169 - Flags: review?(dtownsend+bugmail)
Attachment #777169 - Flags: review?(dtownsend+bugmail) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/23da9889e24f1536ae17c17391e0c5dd3bc8eed3
Merge pull request #1118 from Gozala/hotfix/net-io

Bug 820953 - Implement node style net module. r=@Mossop
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/9a04d3ae17188dada7d857cf88a7447fdf9990f3
Merge pull request #1127 from Gozala/hotfix/net-io

Bug 820953 - Implement nodejs net API a=@gozala
I don't think this need info request is relevant anymore. This problem should be fixed by windowless docless (bug 565388)
Flags: needinfo?(ejpbruel)
Hey Irakli, can we close this now?
Flags: needinfo?(rFobic)
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(rFobic)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.