Closed Bug 714181 Opened 8 years ago Closed 2 years ago

Access to Full Screen API blocked from context-menu API

Categories

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

defect

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: cers, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0a1) Gecko/20111225 Firefox/12.0a1
Build ID: 20111225031006

Steps to reproduce:

Run https://builder.addons.mozilla.org/addon/1031601/revision/1/
or equivalent add-on with main.js:
var cm = require("context-menu");
cm.Item({
  label: "Show in full screen",
  contentScript: "self.on('click', function fullScreenImageContext(n) { n.mozRequestFullScreen(); })"
});

right click any element on a webpage and click "Show in full screen"


Actual results:

Full screen request denied with message in error console:
Warning: Request for full-screen was denied because Element.mozRequestFullScreen() was not called from inside a short running user-generated event handler.


Expected results:

Element should have been full screened.
Drew, can you comment on this? We'll probably hit this more in the future with other things, if this is a problem with our implementation of contentscripts.
Priority: -- → P2
Sorry, I don't know what that warning means.  If it is due to a problem with the SDK's content script implementation as you suggest, like a problem with sandboxes or compartments or principals, I bet Alex and Irakli would know better than me.

It might be helpful to find out whether the same problem happens with page-mod.
First question:
How are we running the "fullScreenImageContext" callback function in the example code in comment 0?

I.e. what code is triggering the call to that function?

Second question:
The function runs in the scope of the webpage itself, right? We don't run it in the jetpack sandbox?


The way that the fullscreen API works is that it only lets you go into fullscreen mode from certain event handlers. I.e. you can go into fullscreen mode from a event listener from the "click" event, but you can't go into fullscreen from a "load" event or from setTimeout callback. Hence the first question above.
(In reply to Jonas Sicking (:sicking) from comment #4)
> Second question:
> The function runs in the scope of the webpage itself, right? We don't run it
> in the jetpack sandbox?
CCing Irakli and Alex to correct me, but as I understand it, the content-script used for the context menu wraps the page's DOM in some way, in order to prevent unprivileged webpage code from gaining access to internal browser privileges.
(In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #5)
> (In reply to Jonas Sicking (:sicking) from comment #4)
> > Second question:
> > The function runs in the scope of the webpage itself, right? We don't run it
> > in the jetpack sandbox?
> CCing Irakli and Alex to correct me, but as I understand it, the
> content-script used for the context menu wraps the page's DOM in some way,
> in order to prevent unprivileged webpage code from gaining access to
> internal browser privileges.

It seems to be something platform related I have reduced the test case to:

Components.classes["@mozilla.org/appshell/window-mediator;1"].
getService(Components.interfaces.nsIWindowMediator).
getMostRecentWindow('navigator:browser').
gBrowser.
contentDocument.
documentElement.
mozRequestFullScreen()

which fails with an error:

[22:08:37.827] Request for full-screen was denied because requesting element is not in the currently focused tab. @ http://www.google.ge/

While error is different I do suspect it's related.
No, that call should behave like that, depending on when you call it.

We need answers to the questions in comment 4. Without that we can't move forward in this bug.
(In reply to Jonas Sicking (:sicking) from comment #4)
> First question:
> How are we running the "fullScreenImageContext" callback function in the
> example code in comment 0?
> 
> I.e. what code is triggering the call to that function?
> 
> Second question:
> The function runs in the scope of the webpage itself, right? We don't run it
> in the jetpack sandbox?
> 

No, content scripts are running in the sandboxes that inherit principals from the webpage. 

> 
> The way that the fullscreen API works is that it only lets you go into
> fullscreen mode from certain event handlers. I.e. you can go into fullscreen
> mode from a event listener from the "click" event, but you can't go into
> fullscreen from a "load" event or from setTimeout callback. Hence the first
> question above.

Also it looks like we do trigger events in the content scripts async (using setTimeout):

https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/context-menu.js#L702
> Also it looks like we do trigger events in the content scripts async (using
> setTimeout):
> 
> https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/
> context-menu.js#L702

That is what's causing this issue. Can we stop doing that?

Otherwise we'll have to add explicit code here to unblock popup-blockers, fullscreen blockers etc (this is a list that will grow with time).

It'd be really nice to not have to duplicate the logic for which events should allow popups/fullscreens/etc which we already have in C++.
It's not window.setTimeout(), it's require("timer").setTimeout(), which is a one-shot nsITimer, right?
The problem isn't using a particular API to make it Async. The problem is making it async at all.

As long as we're not calling into the jetpack sandbox *during* the actual event handling, things like popup blockers and full-screen blockers will behave differently.
Jonas, the reason we emit event asynchronously is because we plan / were planing to migrate jetpack code into a separate process, in which case async message would have been send to a content process. I wonder how that supposed to work with electrolysis ? If there something for electrolysis could we just use that ?
(This appears to have been confirmed by now.)

Forgive my lack of knowledge about Jetpack.

My understanding is that jetpack was intended to have a concept of "in-page scripts" or some such, which was how scripts could access things like the DOM of the page. Since such scripts would be accessing a lot of content in the page itself it was shipped to the content process and executed there.

Is that correct?

*If* my understanding is correct, such "in-page script" wouldn't need to run asynchronously, right?

Or is the intended design that the event is delivered to the jetpack "process" which then has the ability to ship a in-page script to the page and execute it there?
Status: UNCONFIRMED → NEW
Ever confirmed: true
> Or is the intended design that the event is delivered to the jetpack
> "process" which then has the ability to ship a in-page script to the page
> and execute it there?

That was intended design. Regardless, of that my understanding is that click events are dispatched in chrome process which will have to send async message to a different process no matter if it's jetpack process or content one.

Do I miss something ?
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #14)
> > Or is the intended design that the event is delivered to the jetpack
> > "process" which then has the ability to ship a in-page script to the page
> > and execute it there?
> 
> That was intended design.

Makes sense. This means that we'll have to expose some way for the jetpack code to replicate the mode that the popup blocker is in during various events. Maybe an API which lets the jetpack code check the mode when an event is fired, and then set that mode again while running the content script.

Jst: who is working on popup blocker these days?


> Regardless, of that my understanding is that click
> events are dispatched in chrome process which will have to send async
> message to a different process no matter if it's jetpack process or content
> one.

I *think* the way it'll work is that we'll dispatch a new click event in the child process which means that the popup blocker will behave the same way while that event runs.

But that's off-topic to this bug, no?
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.