324907 - scrollbars=no ignored in window.open() (again!)

Status: RESOLVED FIXED

(Camino Graveyard :: Page Layout, defect)

Description

[Wevah (Nate Weaver)]

Comment 1

[Wevah (Nate Weaver)]

Whoa, I forgot to comment. This is bug 232765 all over again, but only on trunk.

Comment 2

[Simon Fraser [no longer active]]

Camino only, or FF too?

Comment 3

[Wevah (Nate Weaver)]

Camino only (just checked).

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We should figure out when this regressed.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Busted on the branch now, too.

Any chance that bug 114850 did this?  (See bz's bug 160627 comment 32)

Targeting for 1.1 since it's a regression, but we need to find the range/cause, too.

Comment 6

[Håkan Waara]

Attachment: Proposed fix

Ever since the security check was introduced in bug 114850, we've been disallowed to change chrome attributes on the newly created window.

Why?  Because the last thing on the JS stack is the JS code from the webpage, which obviously doesn't have the BrowserWriteAccess privilege.

So what we do is to push a null JSContext onto the stack while we create the window. Per Boris' suggestion, of course.

For this to compile, a project file change will be needed on check-in.

Comment 7

[Håkan Waara]

Attachment: Patch v2

I asked Boris to have a look at the patch, and he insisted that I add more error checking (for security reasons) for OOM conditions.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Trunk project patch

MrProject here; trunk patch to add ../dist/include/js to the header search paths

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Branch project patch

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Note that when I built trunk, Wevah's testcases never finished loading (progress bar continues to go, and the second testcase never lets me scroll); branch was fine.

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 237230 [details] [diff] [review]
Patch v2

>Index: src/embedding/CHBrowserService.mm
>===================================================================

>+  rv = stack->Push(nsnull);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>   nsCOMPtr<nsIWindowCreator> browserChrome(do_QueryInterface(parent));
>   return browserChrome->CreateChromeWindow(parent, chromeFlags, _retval);
>+  
>+  JSContext *ctx;
>+  stack->Pop(&ctx);
>+  NS_ASSERTION(!ctx, "Null JSContext-hack in Camino not working!");

Is this a 'return' I see before me?

Comment 12

[Håkan Waara]

Attachment: Patch v3

Oops, thanks for catching that Simon! :)

Comment 13

[Håkan Waara]

Comment on attachment 237306 [details] [diff] [review]
Patch v3

Grr, actually, maybe I should cache the rv of CreateChromeWindow and return at the end. It's done in my local tree now.

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 237306 [details] [diff] [review]
Patch v3

r=me with the rv propagation.

We also use this hack somewhere else; maybe we should write a stack-based C++ utility class that does it?

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 237306 [details] [diff] [review]
Patch v3

This patch also works properly on the trunk now. :)

Comment 16

[Håkan Waara]

(In reply to comment #14)
> (From update of attachment 237306 [details] [diff] [review] [edit])
> r=me with the rv propagation.
> 
> We also use this hack somewhere else; maybe we should write a stack-based C++
> utility class that does it?

That's a good idea. I created such a helper class, but it struck me that we can't propagate errors then (without throwing, and we don't do exceptions). Any idea of a solution?

Comment 17

[Håkan Waara]

Attachment: Fix w/ JSContextHack helper class

Here's a fix with a new stack-based class |JSContextHack| that makes our code look a bit nicer.

Comment 18

[Simon Fraser [no longer active]]

Comment on attachment 237478 [details] [diff] [review]
Fix w/ JSContextHack helper class

>Index: src/embedding/CHBrowserService.mm
>===================================================================

>+  // Push a null JSContext on the JS stack, before we create the chrome window.
>+  // Otherwise, a webpage invoking some JS to do window.open() will be last on the JS stack.
>+  // And once we start fixing up our newly created chrome window (to hide the scrollbar,
>+  // for example) Gecko will think it's the *webpage*, and webpages are not allowed
>+  // to do that.  see bug 324907.
>+  nsresult rv = NS_OK;
>+  JSContextHack hack(&rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>   nsCOMPtr<nsIWindowCreator> browserChrome(do_QueryInterface(parent));
>-  return browserChrome->CreateChromeWindow(parent, chromeFlags, _retval);
>+  rv = browserChrome->CreateChromeWindow(parent, chromeFlags, _retval);
>+  
>+  return rv;

You can just return browserChrome-> .... here. The C++ dtor will still happen after the CreateChromeWindow call returns.

>Index: src/extensions/GeckoUtils.h
>===================================================================

>+/* Stack-based utility that will push a null JSContext onto the JS stack during the
>+   length of its lifetime. */

I think the commment needs to explain why this is used (explaining the security implications).

>+struct JSContextHack {

Make it a class.
Also, it needs a better name, like StNullJSContextPusher or something.

> 
>+  JSContextHack(nsresult *rv) {
>+    mStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1", rv);
>+    if (NS_SUCCEEDED(*rv) && mStack)
>+      *rv = mStack->Push(nsnull);
>+  }
>+  
>+  ~JSContextHack() {
>+    if (mStack) {
>+      JSContext *ctx;
>+      mStack->Pop(&ctx);

Assert that ctx is null here, to catch mismatched push/pops.

>+    }
>+  }
>+  
>+private:
>+  nsCOMPtr<nsIJSContextStack> mStack;
>+};
>+

Comment 19

[Håkan Waara]

Attachment: Patch v3

This patch addresses all the review comments. I tried to avoid a class name involving "pusher", due to its other possible meanings ;)

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 237513 [details] [diff] [review]
Patch v3

sr=me but please prefix the class name with "St" to indicate that it's intended as a stack-based class.

Comment 21

[Håkan Waara]

Fixed!