Last Comment Bug 324907 - scrollbars=no ignored in window.open() (again!)
: scrollbars=no ignored in window.open() (again!)
Status: RESOLVED FIXED
: fixed1.8.1, regression
Product: Camino Graveyard
Classification: Graveyard
Component: Page Layout (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: Håkan Waara
:
:
Mentors:
http://www.derailer.org/temp/camino_w...
Depends on:
Blocks: 160627
  Show dependency treegraph
 
Reported: 2006-01-26 23:27 PST by Wevah
Modified: 2006-09-09 22:26 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (2.47 KB, patch)
2006-09-07 16:35 PDT, Håkan Waara
no flags Details | Diff | Splinter Review
Patch v2 (2.57 KB, patch)
2006-09-07 16:52 PDT, Håkan Waara
sfraser_bugs: review-
Details | Diff | Splinter Review
Trunk project patch (7.00 KB, patch)
2006-09-07 17:50 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review
Branch project patch (7.18 KB, patch)
2006-09-07 17:51 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review
Patch v3 (2.64 KB, patch)
2006-09-08 01:23 PDT, Håkan Waara
sfraser_bugs: review+
Details | Diff | Splinter Review
Fix w/ JSContextHack helper class (6.78 KB, patch)
2006-09-09 05:07 PDT, Håkan Waara
sfraser_bugs: superreview-
Details | Diff | Splinter Review
Patch v3 (7.07 KB, patch)
2006-09-09 11:20 PDT, Håkan Waara
sfraser_bugs: superreview+
Details | Diff | Splinter Review

Description User image Wevah 2006-01-26 23:27:02 PST
 
Comment 1 User image Wevah 2006-01-27 00:31:45 PST
Whoa, I forgot to comment. This is bug 232765 all over again, but only on trunk.
Comment 2 User image Simon Fraser 2006-01-27 07:39:13 PST
Camino only, or FF too?
Comment 3 User image Wevah 2006-01-27 16:08:39 PST
Camino only (just checked).
Comment 4 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-05 01:47:45 PDT
We should figure out when this regressed.
Comment 5 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-06 17:52:45 PDT
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 User image Håkan Waara 2006-09-07 16:35:06 PDT
Created attachment 237219 [details] [diff] [review]
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 User image Håkan Waara 2006-09-07 16:52:38 PDT
Created attachment 237230 [details] [diff] [review]
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-07 17:50:48 PDT
Created attachment 237243 [details] [diff] [review]
Trunk project patch

MrProject here; trunk patch to add ../dist/include/js to the header search paths
Comment 9 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-07 17:51:30 PDT
Created attachment 237244 [details] [diff] [review]
Branch project patch
Comment 10 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-07 17:52:34 PDT
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 User image Simon Fraser 2006-09-07 21:31:17 PDT
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 User image Håkan Waara 2006-09-08 01:23:03 PDT
Created attachment 237306 [details] [diff] [review]
Patch v3

Oops, thanks for catching that Simon! :)
Comment 13 User image Håkan Waara 2006-09-08 01:26:44 PDT
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 User image Simon Fraser 2006-09-08 08:53:48 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-08 09:34:46 PDT
Comment on attachment 237306 [details] [diff] [review]
Patch v3

This patch also works properly on the trunk now. :)
Comment 16 User image Håkan Waara 2006-09-08 10:20:39 PDT
(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 User image Håkan Waara 2006-09-09 05:07:15 PDT
Created attachment 237478 [details] [diff] [review]
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 User image Simon Fraser 2006-09-09 10:30:57 PDT
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 User image Håkan Waara 2006-09-09 11:20:43 PDT
Created attachment 237513 [details] [diff] [review]
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 User image Simon Fraser 2006-09-09 11:22:45 PDT
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 User image Håkan Waara 2006-09-09 11:59:23 PDT
Fixed!

Note You need to log in before you can comment on or make changes to this bug.