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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Page Layout
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Wevah, Assigned: Håkan Waara)

Tracking

(Blocks: 1 bug, {fixed1.8.1, regression})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, regression

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
 
(Reporter)

Comment 1

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

Comment 2

12 years ago
Camino only, or FF too?
(Reporter)

Comment 3

12 years ago
Camino only (just checked).
We should figure out when this regressed.
Component: General → Page Layout
Keywords: qawanted
QA Contact: page.layout
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.
Summary: scrollbars=no ignored in window.open() (again!) on trunk → scrollbars=no ignored in window.open() (again!)
Target Milestone: --- → Camino1.1
Version: Trunk → unspecified
Blocks: 160627
(Assignee)

Comment 6

11 years ago
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.
Attachment #237219 - Flags: review?(mikepinkerton)
(Assignee)

Comment 7

11 years ago
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.
Attachment #237219 - Attachment is obsolete: true
Attachment #237230 - Flags: review?(mikepinkerton)
Attachment #237219 - Flags: review?(mikepinkerton)
Created attachment 237243 [details] [diff] [review]
Trunk project patch

MrProject here; trunk patch to add ../dist/include/js to the header search paths
Attachment #237243 - Flags: review?
Created attachment 237244 [details] [diff] [review]
Branch project patch
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

11 years ago
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?
Attachment #237230 - Flags: review?(mikepinkerton) → review-
(Assignee)

Comment 12

11 years ago
Created attachment 237306 [details] [diff] [review]
Patch v3

Oops, thanks for catching that Simon! :)
Attachment #237230 - Attachment is obsolete: true
Attachment #237306 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #237306 - Attachment is patch: true
Attachment #237306 - Attachment mime type: application/octet-stream → text/plain
Attachment #237306 - Flags: review? → review?(sfraser_bugs)
(Assignee)

Comment 13

11 years ago
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

11 years ago
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?
Attachment #237306 - Flags: review?(sfraser_bugs) → review+
Comment on attachment 237306 [details] [diff] [review]
Patch v3

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

Comment 16

11 years ago
(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?
(Assignee)

Comment 17

11 years ago
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.
Assignee: sfraser_bugs → hwaara
Attachment #237306 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #237478 - Flags: superreview?(sfraser_bugs)

Comment 18

11 years ago
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;
>+};
>+
Attachment #237478 - Flags: superreview?(sfraser_bugs) → superreview-
(Assignee)

Comment 19

11 years ago
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 ;)
Attachment #237478 - Attachment is obsolete: true
Attachment #237513 - Flags: superreview?(sfraser_bugs)

Comment 20

11 years ago
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.
Attachment #237513 - Flags: superreview?(sfraser_bugs) → superreview+
(Assignee)

Comment 21

11 years ago
Fixed!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Keywords: qawanted
Attachment #237243 - Flags: review?
You need to log in before you can comment on or make changes to this bug.