619991 - Panel content resizing firefox window

Status: RESOLVED FIXED

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

Description

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: A testcase

I found this bug when trying to open the Facebook Share page inside an small panel and having firefox not maximized. When the page inside the panel rendered, it started to resize the firefox window until it was maximized.

I attach a main.js testcase.

The testcase creates two widgets, one that reproduces the bug ("Resizing") and one that does some kind of workaround. The workaround is to set the panels height/width to a bigger size than facebook expects, and then the firefox window is not resized.

Comment 1

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

Alex: could you take a look at this bug and see if you can figure out what's going on?

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Unittest with a Panel resizing browser window via window.resizeTo

UL iframe with type="content" doesn't block resize and move javascript functions:
resizeTo, resizeBy, moveTo and moveBy; because web content is able to play with browsers windows.

The only known way is to add another iframe, which is going to block these calls.
Sub iframes in a web page can't change browser window position nor size.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: New educational unit test that highlight resize/move function blocking solutions.

Here is a plain new unit test file, that show how to block the resize/move functions:
One XUL iframe with type="content" that load one child (xul or html) iframe with the content we want to load.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Fix for this bug by adding a new child html iframe in the former XUL one

Here is a way to solve this bug, by adding one additional iframe in Symbiont.

I have two points to review with you:
1/ I'm not sure it is the best location, I may implement one another moduble on top of Symbiont with this new concept of `security frame` and `content frame`. This would be a module to create *very-safe* iframes, all createElement will be called within this module. This module will be used by modules that create new iframes like Panel/Widget but not with modules that work with browser iframes.

2/ There is an hack that may broke stuff. As we load our content in a sub frame of an type="content" iframe, we can't load chrome/resource url! I used XMLHttpRequest in symbiont.js to fix this problem. But these chrome documents won't have chrome privilieges anymore.

I ask to bz about a platform solution to block these calls, but it doesn't seems to exist and he suggested me to fill a bug with a complete description of iframe we want. That being said, we may want to block extra stuff, so we need some extra thinking on what is currently authorized for web content but could be interesting to block in our iframes.

Comment 5

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

Comment on attachment 499832 [details] [diff] [review]
Unittest with a Panel resizing browser window via window.resizeTo

Looks good, works as expected. r=myk

Comment 6

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

Comment on attachment 499839 [details] [diff] [review]
Fix for this bug by adding a new child html iframe in the former XUL one

(In reply to comment #4)
> 1/ I'm not sure it is the best location, I may implement one another moduble on
> top of Symbiont with this new concept of `security frame` and `content frame`.
> This would be a module to create *very-safe* iframes, all createElement will be
> called within this module. This module will be used by modules that create new
> iframes like Panel/Widget but not with modules that work with browser iframes.

Hmm, I'm not sure that's strictly necessary.  What would be the benefits of this approach?


> 2/ There is an hack that may broke stuff. As we load our content in a sub frame
> of an type="content" iframe, we can't load chrome/resource url! I used
> XMLHttpRequest in symbiont.js to fix this problem. But these chrome documents
> won't have chrome privilieges anymore.

This may actually be a feature rather than a bug, since our goal is to make chrome access explicit by requiring addons to "require('chrome')" in order to get it, and addons shouldn't be able to bypass that requirement by loading a chrome URL in a panel.


> I ask to bz about a platform solution to block these calls, but it doesn't
> seems to exist and he suggested me to fill a bug with a complete description of
> iframe we want. That being said, we may want to block extra stuff, so we need
> some extra thinking on what is currently authorized for web content but could
> be interesting to block in our iframes.

Yeah, we should definitely think about that.  At the same time, I don't think we need to bundle these requests together.  It'll probably be more productive to ask for each additional frame feature as we identify it, i.e. to file a bug requesting this feature now, then file additional bugs as we identify additional features.


> +    item.symbiont.on('iframeReady', function onIframeReady(){

Nit: add a space before the opening brace ({), i.e.:

    item.symbiont.on('iframeReady', function onIframeReady() {


> +    console.log("Testing : "+widgetOptions.label);

Nit: surround plus operators with spaces, i.e.:

    console.log("Testing : " + widgetOptions.label);

But I wonder, is this log message really necessary?  It seems like temporary debugging code that should be removed in the final patch (or changed to "console.debug()").


> +const Symbiont = Worker.resolve({ constructor: '_onInit' }).compose(Loader,{

Ah, yes, it's much better to put Loader as the first item in the composition, as that makes it much more noticeable to someone reading through the code!

Nit: insert a space before opening brace, i.e.:

  const Symbiont = Worker.resolve({ constructor: '_onInit' }).compose(Loader, {


> +      function onFrameReady(event) {
> +        frame.removeEventListener(ON_READY, onFrameReady,true);
> +        
> +        
> +      }, 

Nit: add space before "true".
Nit: remove extraneous lines.


> +      } else {
> +        observers.add(ON_START, self._onStart = self._onStart.bind(self));

Nit: don't cuddle else, i.e.:

      }
      else {
        observers.add(ON_START, self._onStart = self._onStart.bind(self));


> +      console.log("sub src = "+(subSrc?subSrc:'data:text/html,'));

Nit: put spaces around operators, i.e.:

      console.log("sub src = " + (subSrc ? subSrc : 'data:text/html,'));


+      let html = '<iframe id="html-frame" src="'+(subSrc?subSrc:'data:text/html,')+'" '
+               + 'style="position:absolute;top:0;bottom:0;right:0;left:0;'
+               + '       border:0;" /><!--'+(new Date().getTime())+'-->';

Nit: put spaces around operators here too.


> +    if (!this._frame.contentDocument) return;

Nit: put conditional blocks on the next line, i.e.:

    if (!this._frame.contentDocument)
      return;


Otherwise, this looks ok.  I'm concerned about adding this kind of hack, but I don't see a better option.  We could just throw up our hands and say that this is a problem for addon developers to solve for themselves (i.e. by not loading such content into a panel), but that doesn't seem like a better solution.

Nevertheless, when I run the tests after applying this patch, I still see failures in the testcases for this functionality.  So this doesn't seem to be doing what it intends to do.

Comment 7

[Hernán Rodriguez Colmeiro (:peregrino)]

Any news on this bug? Also, I think Bug 615433 might be related, or even be a dupe.

Comment 8

[Alexandre Poirot [:ochameau]]

I'm even more concerned about applying such patch to this already enough complex Symbiont, Widget code.
So I filed Bug 635673 to request a new feature from platform to block these resizes. I'd like to see platform team response before being forced to apply this patch.

Comment 9

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

This needs the platform fix.  Once it lands, we'll need to update the SDK code to use it.

Comment 10

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

(automatic reprioritization of 1.0 bugs)

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Use new allowWindowControl attribute

This new docshell attribute works perfectly!
So I think it makes sense to include it right now.
It is only available on Nightly but it should be available in Aurora on July 5th, and finally in Firefox 7.

So I don't know how to procede next? Do we keep this bug opened? Do we execute this test by cheking dynamically platform version?

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 543180 [details] [diff] [review]
Use new allowWindowControl attribute

Review of attachment 543180 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's better to do feature test as suggested in comment instead of commenting out. Regardless change looks good to me and I trust you can make a suggested change without additional review cycle, but please make sure to test ;)

::: packages/addon-kit/tests/test-panel.js
@@ +83,5 @@
>  };
>  
> +/*
> + * TODO: Enable this test when bug 635673: dochell.allowWindowControl 
> + * is shipped in an stable Firefox release, should be Firefox 7.

I think it will be better to perform same check as above:

if ("allowWindowControl" in window.docShell) return test.pass(...)

Comment 13

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/01555930c6d9bc04d9d0650ae52148a601db987d