Closed
Bug 820953
Opened 12 years ago
Closed 10 years ago
Expose DOM window to the add-on
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: irakli, Assigned: irakli)
References
Details
Attachments
(8 files)
(deleted),
text/html
|
evold
:
review-
mossop
:
review+
|
Details |
(deleted),
text/html
|
ochameau
:
review+
|
Details |
(deleted),
text/html
|
zer0
:
review+
|
Details |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
mossop
:
review+
|
Details |
(deleted),
text/html
|
mossop
:
review+
|
Details |
(deleted),
text/html
|
mossop
:
review+
|
Details |
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!!
Assignee | ||
Comment 1•12 years ago
|
||
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
Comment 2•12 years ago
|
||
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.
Comment 3•12 years ago
|
||
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?
Comment 4•12 years ago
|
||
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.
Priority: -- → P1
Assignee | ||
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
> 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. :(
Assignee | ||
Comment 7•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 8•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #694584 -
Flags: review?(evold)
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
>
> 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.
Assignee | ||
Comment 11•12 years ago
|
||
(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 ?
Comment 12•12 years ago
|
||
> 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 13•12 years ago
|
||
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-
Blocks: 781486
Updated•12 years ago
|
Flags: needinfo?(dtownsend+bugmail)
Attachment #694584 -
Flags: review?(dtownsend+bugmail)
Updated•12 years ago
|
Flags: needinfo?(dtownsend+bugmail)
Comment 14•12 years ago
|
||
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-
Updated•12 years ago
|
Attachment #694584 -
Flags: review- → review+
Comment 15•12 years ago
|
||
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
Backed out in https://github.com/mozilla/addon-sdk/commit/670279994b4c49cc71960c022165b96de41f0d2e for blowing almost everything up.
Assignee | ||
Comment 17•12 years ago
|
||
## 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)
Assignee | ||
Comment 18•12 years ago
|
||
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)
Comment 19•12 years ago
|
||
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)
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
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.
Assignee | ||
Comment 22•12 years ago
|
||
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.
Assignee | ||
Comment 23•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 24•12 years ago
|
||
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)
Comment 25•12 years ago
|
||
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 26•12 years ago
|
||
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 27•12 years ago
|
||
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+
Comment 28•12 years ago
|
||
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
Comment 29•12 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk
https://github.com/mozilla/addon-sdk/commit/86c5fcd61c8c883f2d80d43256ba66d65b626591
Hotfix regression introduced in previous commit: Bug 820953
Comment 30•12 years ago
|
||
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
Assignee | ||
Comment 31•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•12 years ago
|
Attachment #749280 -
Flags: review?(zer0)
Comment 32•11 years ago
|
||
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+
Comment 33•11 years ago
|
||
+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...
Comment 34•11 years ago
|
||
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
Assignee | ||
Comment 35•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #757562 -
Flags: review?(dtownsend+bugmail)
Blocks: 880558
Comment 36•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #757562 -
Flags: review?(dtownsend+bugmail)
Comment 37•11 years ago
|
||
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)
Comment 38•11 years ago
|
||
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
Assignee | ||
Comment 40•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 41•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #767503 -
Flags: review?(dtownsend+bugmail)
Comment 42•11 years ago
|
||
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+
Comment 43•11 years ago
|
||
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
And back out again :(
https://github.com/mozilla/addon-sdk/commit/8e6c7643e1c60b5bee2b246cba42883e400775c6
Assignee | ||
Comment 45•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #772942 -
Flags: review?(dtownsend+bugmail)
Comment 46•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #772942 -
Flags: review?(dtownsend+bugmail) → review+
Comment 48•11 years ago
|
||
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
Assignee | ||
Comment 49•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #777169 -
Flags: review?(dtownsend+bugmail)
Updated•11 years ago
|
Attachment #777169 -
Flags: review?(dtownsend+bugmail) → review+
Comment 50•11 years ago
|
||
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
Comment 51•11 years ago
|
||
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
Depends on: 897683
Comment 52•11 years ago
|
||
I don't think this need info request is relevant anymore. This problem should be fixed by windowless docless (bug 565388)
Flags: needinfo?(ejpbruel)
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(rFobic)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•