408052 - Adopt "descendant" frame navigation policy to prevent frame hijacking

Status: VERIFIED FIXED

(Core :: Security, defect, P2)

Description

[Adam Barth]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11

Many security-sensitive pages, such as login pages, contain inline frames (iframes). For example, the password-entry field on Google AdSense, Hushmail, and several bank web sites are contained in iframes. These frames appear to be part of the parent page and do not have address bars (or any kind of security indicator). Because the user has no visible indication of the source of the content that appears in the iframe, the user implicitly trusts the parent page to fill the iframe with trustworthy content. Protecting the integrity of the frame's contents is critical to the security of these sites.

Firefox has a more permissive frame navigation policy than IE and Safari, allowing an active frame to navigate a target frame if (1) the target frame is a top-level frame or (2) the active frame can script any frame in the same window as the target frame.

This policy permits frame hijacking attacks:

 1. Sites such as iGoogle or Live.com contain a number of iframe "gadgets." A malicious gadget can navigate all the other gadgets in the same window, tricking a user who is expecting to interact with some trusted gadgets (such as a Chat or Email gadget).

 2. Sites commonly display third-party advertisements in iframes. A malicious advertisement can navigate the other ad frames to steal their advertising impression and display advertisements of the attacker's choice.

Firefox should implement the same policy as IE and Safari:

An active frame can navigate a target frame if (1) the target frame is a top-level frame or (2) the active frame can script the target frame or any of its ancestors in the frame hierarchy.

A full discussion of frame navigation policies in Firefox, IE, Safari, and Opera is available at <http://crypto.stanford.edu/frames/>.


Reproducible: Always

Steps to Reproduce:
1. Go to <http://www.google.com/ig>.  (Logging in is optional.)
2. Add some non-inline gadgets; for example a Hotmail gadget.  (There is currently already non-inline YouTube gadget on the default iGoogle page.)
3. Add the attacker's gadget by visiting <http://www.google.com/ig/adde?synd=open&source=ggyp&moduleurl=http://crypto.stanford.edu/frames/igoogle/attacker.xml>
4. Click the "Navigate my gadgets!" button.

Actual Results:  
The contents of non-inline gadgets are replaced with content of the attacker's choice (in this case Yahoo!'s homepage).

Expected Results:  
The attacker is only able to "hijack" his own gadget.  This is the behavior of IE7 and the latest WebKit nightly builds.

Comment 1

[Adam Barth]

Attachment: Matches the IE7 and Safari frame navigation policy

Here is a patch that implements the Ancestor frame navigation policy, matching IE7 and Safari.

Comment 2

[Jesse Ruderman]

http://crypto.stanford.edu/frames/ is very detailed and helpful.  Thanks!

There are two major methods for navigating other frames: named targeting and direct navigation.  I'm not sure we currently use the same policy for both.  Your page refers to bug 13871 and bug 103638, which were primarily about named targeting, while your testcase uses direct navigation.  IIRC, the current behavior for direct navigation is mostly the result of patches in bug 52920 and bug 246448.

We adopted the permissive policy for direct navigation in bug 52920 for web compatibility reasons, despite my security concerns (bug 52920 comment 6).  But IE7 adopted a more restricted policy and took the brunt of the web-compatibility hit (go Microsoft!), so we should be able to do the same now.

Comment 3

[Collin Jackson]

Attachment: Frame navigation test results for unpatched Firefox

We put together a test case of frame navigation within a single window using window.open(). The test case is intended to cover the essential domain and parent/child combinations that can occur within a single page.

Our test case can be found at <http://w3sim.com/frames/>.

The attached screenshot shows the behavior of Firefox 2.0.0.11 on the test case.

Comment 4

[Collin Jackson]

Attachment: Frame navigation test results for IE7

This screenshot shows IE7's behavior on our test case. The red text indicates frame navigations that failed, creating a popup instead of navigating the targeted iframe.

Comment 5

[Collin Jackson]

Attachment: Frame navigation test results for Safari

The WebKit component of Safari has recently adopted the same frame navigation policy as IE7, as shown in this screenshot of a recent nightly build.

Comment 6

[Collin Jackson]

Attachment: Frame navigation test results for patched Firefox

Our patch updates Firefox to match the behavior of IE7 and Safari on our test case, as shown in the attached screenshot.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Firefox has a more permissive frame navigation policy than IE and Safari,
> allowing an active frame to navigate a target frame if (1) the target frame is
> a top-level frame or (2) the active frame can script any frame in the same
> window as the target frame.

By "frame navigation" do you mean named window targeting?  Or something else?  That's the first place to start.  Different policies are applied to named windows and to other navigation methods.

Comment 8

[Adam Barth]

IE7 uses the same policy for window.open, hyperlinks with targets, and setting the location property:

Test case for hyperlinks with targets:
  http://xenon.stanford.edu/~abarth/research/nav/frame1.html

Test case for window.open:
  http://xenon.stanford.edu/~abarth/research/nav-open/frame1.html

Test case for setting location property:
  http://xenon.stanford.edu/~abarth/research/nav-pointer/frame1.html

Although Firefox uses different policies for different types of navigation, it makes sense to enforce a consistent security policy across all of them.

As Jesse pointed out, the patch does not yet address navigation via setting the location property, but we think that doing so would be a good idea.

Comment 9

[Jesse Ruderman]

It's possible that we'll want targeted loads to be "stricter" than setting the location property, not for security reasons but to match the HTML5 spec or expected web site behavior.

Comment 10

[Collin Jackson]

We have brought up the issue in the HTML5 working group and it seems likely that if Firefox synchronizes its policy with the IE7 and Safari policy, the HTML5 spec will adopt this policy as well.

Comment 11

[Boris Zbarsky [:bzbarsky]]

The policy for targeted loads is somewhat "looser" in that you can target windows that are not reachable though JS (e.g. windows opened via window.open() that you then didn't hold a reference to).

> IE7 uses the same policy for window.open, hyperlinks with targets, and setting
> the location property:

Is this documented somewhere (same question as what I asked in comment ?  Or are the testcases cited in comment 8 exhaustive?

Basically, we need to have a clear spec for this stuff before we start changing it; bit-by-bit changes will just cause pain for testers and web developers and lead to confusing code.  I see no reason to treat window.location and named targets identically (since per above they are not in fact identical, and since in practice the use cases are quite different).

If we're just talking named targets and there is a proposed patch, could someone attach a readable (-up12 is good) version of it?

Comment 12

[Hixie (not reading bugmail)]

The rules for window.open() and hyperlinks with targets in HTML5 attempt to resolve the security issues raised in this bug:

   http://www.whatwg.org/specs/web-apps/current-work/#the-rules

I would be interested in knowing what should change in those rules if they are not adequate (i.e. do you think you could implement them? If not, what should I change so that you would be willing to implement them?).

Comment 13

[Collin Jackson]

The policy described in the HTML5 spec is the Parent policy described in our white paper: <http://crypto.stanford.edu/frames/>

From a security standpoint, the Parent policy is fine with us, because it is more restrictive than Ancestor. We consider the Ancestor policy to be more convenient for two reasons:

1) Ancestor already has adoption in two browsers. Any web sites that are not compatible with Ancestor have already had a year to fix their sites for IE7. Opera attempts to implement Parent, but does not do so correctly for window.open(). Because no major browser implements Parent according to the HTML5 spec, it is not known whether the Parent would break functionality in a large number of legitimate sites.

2) Ancestor permits more functionality while admitting no known attacks. For example, Ancestor permits more flexible fragment identifier messaging.

Comment 14

[Adam Barth]

Attachment: Updated patch to restrict setting location property also

> I see no reason to treat window.location and named targets identically

Currently, nsDocShell::CheckLoadingPermissions (which checks window.location) and nsDocShell::CanAccessItem (which checks named targets) enforce the same navigation policy, except that nsDocShell::CanAccessItem is restricts by opener.  However, the opener restriction is easily circumvented unless the web site takes preventative measures that only work in Firefox.

The attached patch changes the security checks for both nsDocShell::CheckLoadingPermissions and nsDocShell::CanAccessItem to enforce the Ancestor policy.

Comment 15

[Hixie (not reading bugmail)]

The "Ancestor policy" is vulnerable to the attack vector I described in my e-mail reply to your e-mail: if a site acts differently based on what page (from its own site) is loaded in an iframe in its page, then it would be bad if an evil site could load the whole thing in an iframe itself and then manipulate the inner iframe to trick the site into doing something different.

I really see no good reason to allow a third-party domain to affect iframes that it neither created nor was loaded in.

Comment 16

[Collin Jackson]

It's clear that either Ancestor or Parent are better than the current (Window) policy. Both the Ancestor and Parent policies prevent real attacks on real sites that are currently allowed by the Window policy, e.g. iGoogle gadget hijacking.

Both the Ancestor and Parent policies allow a parent frame to overwrite some or all of the child frame's screen area with malicious content -- these attacks are trivial under either policy, but the child can always detect whether it's in an iframe by checking if top === self and framebusting if necessary.

Advantages of Parent policy: Stricter, simpler, provides more isolation to frames that don't trust their parent but aren't willing to framebust.

Advantages of Ancestor policy: Because a frame's ancestor can overwrite the frame's screen real estate (under both Parent and Ancestor policies), the Ancestor policy is more consistent with the "don't prohibit what you can't prevent" philosophy, avoiding a false sense of security among developers who build iframes that don't trust the parent page. It may be more compatible with existing web site designs since the stricter Parent policy has never been deployed on a large scale.

Either policy seems secure enough, but we should be sure to choose something that all browser vendors can agree on so we don't have settle for the worst of both worlds.

Comment 17

[Damon Sicore (:damons)]

+'ing this and making it a P2.  Let's decide if we should take this.

Comment 18

[Collin Jackson]

Apple deployed the Ancestor policy today in Safari as part of Security Update 2007-009 <http://docs.info.apple.com/article.html?artnum=307179>.

Comment 19

[Collin Jackson]

Attachment: Updated patch with mochitest

We added a mochitest that checks the browser's frame navigation policy for three navigation methods: setting window.location, using window.open, and submitting a form that targets a named frame. We don't expect any major compatibility issues because this patch changes Firefox to match IE7, but we still think it's a good idea to give it some time to bake in the nightly just in case.

Comment 20

[Collin Jackson]

Attachment: Updated patch with improved mochitest

There was a bug in the earlier mochitest that I've now corrected. If the first navigation attempt results in an exception being thrown, the exception will now be caught and the remaining attacks are also tested.

Comment 21

[Boris Zbarsky [:bzbarsky]]

That patch regresses one of the bugs that got fixed in this code at some point.  In particular, targeting all toplevel windows is not acceptable.  Sadly, we haven't gotten all the tests for the various bugs we fixed here into mochitest yet; we really need to.

In fact, I'd call getting regression tests for all the bugs this code has historically fixed in a prerequisite for changing the code; we really don't want to regress those bugs.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Though I guess Adam mentioned this in comment 14 when he said "However, the opener restriction is easily circumvented unless the web site takes preventative measures that only work in Firefox."

I'm not sure I follow that.  The opener restriction is so that you can't target random windows opened by other sites (e.g. banks' login popups).  Now that we always show the URL bar it's somewhat mitigated, but we should still prevent this behavior.  There is no special effort on the bank's part that's needed here.

Comment 23

[Adam Barth]

We removed the opener restriction on purpose because an active frame can bypass this restriction and navigate any top-level frame in the same unit of related browsing contexts:

   1. An active frame can invade the root frame of a related browsing context by
         1. dereferencing window.top to get a reference to a top-level frame,
         2. dereferencing the opener property of that frame and its openers until it reaches the root, and
         3. setting the root's location property.
   2. The root frame of the browsing context can navigate any open windows by recursively using window.open to navigate windows to its own origin. (Using history.go(-1), any of these windows can be returned to their previous state.)

Sites can sever ties with related browsing contexts by opening only anonymous windows and by setting their window.opener to null. However, these countermeasures are not widely used today and may impair legitimate web functionality.

We think it make sense to remove the opener restriction (matching IE7 and Safari), but we think it's more important to tighten down frame navigation within a window, allowing people to build gadget aggregators like iGoogle.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> in the same unit of related browsing contexts

This is important.  Your change allows a frame to also target unrelated browsing contexts.

Specifically, with your change if I as a user open a new window from the file menu, then click on a bookmark link to load my bank site in the window, then click a link on the resulting page that brings up a login window, then any other window I ever opened can override the contents of that login window.  That's not acceptable.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 2. dereferencing the opener property of that frame and its openers until it
> reaches the root, and

Oh, and as far as this goes, you can only do that if you're same-origin with everything you traverse as you do this.

I agree that it's important to protect against frame navigation abuses within a window, but I don't think doing so requires removing the cross-window protection that's already in place.

Comment 26

[Collin Jackson]

Attachment: Opener restriction circumvention example

> Oh, and as far as this goes, you can only do that if you're same-origin with
> everything you traverse as you do this.

The opener property isn't constrained by the same-origin policy, so it's possible to traverse the opener hierarchy (e.g. opener.opener) without being in the same origin as another page. You can also temporarily coerce the page into your own origin by setting its location and then navigating it back to its old one with history.go(-1).

Here's an demonstration of the opener circumvention attack technique that Adam described:
http://crypto.stanford.edu/~collinj/research/firefox/opener/

I've attached the source code.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> The opener property isn't constrained by the same-origin policy

Ah, indeed.  Perhaps that needs to change, or perhaps we the interaction of opener and origin changes needs rethinking (in a separate bug).  In any case, this is minor compared to the unrelated browsing context issue.

Comment 28

[Collin Jackson]

Attachment: Updated patch that leaves opener restriction intact

Here is a version of the patch that leaves the opener restriction in place. 

It is true that the opener restriction may have some use as a defense in depth measure, although I do worry that it gives a false sense of security. However, our most important concern should fixing the vulnerabilities in frame navigation within a single window, so let's focus on the main issue and we can postpone discussion of the opener restriction for a future bug.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Just to make sure we're on the same page, which bugs' testcases have you tested with this patch to make sure you're not regressing them?

Comment 30

[Collin Jackson]

Comment on attachment 295997 [details] [diff] [review]
Updated patch that leaves opener restriction intact

The bugs we've seen that have test cases available are bug 13871, bug 20682, bug 246448 (including the Secunia advisory), and bug 273699. We can convert these into mochitests to help identify regressions. Bug 246923 don't have any attached test cases but we'll try to cover the issues discussed there. Bug 103638 focuses on the frame search process rather than the frame navigation policy. I'll clear the review flag while we work on this.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I think bug 103638 is what I was thinking of with the opener thing.  The frame search process aand navigation policy are one and the same: if you can find it, you can navigate it.

If you're willing to write tests for those, that would be absolutely marvelous.  

If you are also willing to do the various regressions those changes caused (bug 270414, bug 278143, bug 278916 [might be hard to test in Firefox], bug 279495, etc) that would be even lovelier, but I can understand if you have finite amounts of time on your hands.  ;)  Please do make sure you don't regress those, though.

Comment 32

[Collin Jackson]

Attachment: Updated patch with additional mochitests

Adam and I developed a set of mochitests for identifying frame navigation regression issues.

test_bug13871.html - tests classic frame hijacking: bug 13871, bug 20682, and bug 273699
test_bug270414.html - tests bug 270414
test_bug278916.html - tests bug 278916
test_bug278143.html - tests bug 278143, bug 279495
test_child.html - tests basic functionality of navigating child iframes
test_grandchild.html - distinguishes between ancestor and parent policy
test_not-opener.html - tests opener restriction
test_opener.html - tests basic functionality of navigating popup windows
test_reserved.html - tests frame hijacking of parents, reserved names, parts of bug 246448
test_sibling-matching-parent.html - tests parts of bug 246448
test_sibling-off-domain.html - tests for frame hijacking of siblings, parts of bug 246448

For each arrangement of frames, we test navigation using window.location, window.open, form submission, and targeted hyperlinks.

In the process of developing test_reserved.html we found a bug in our original patch and fixed it. The current build fails test_sibling-off-domain.html and half of test_reserved.html -- these are the attacks that our patch fixes.

Comment 33

[Adam Barth]

Attachment: Updated patch with mochitests

Cleaned up some unused code in the mochitests.

Comment 34

[Collin Jackson]

Attachment: Updated patch with mochitests

Cleaned up the mochitest code a bit more, and fixed a Makefile bug.

Comment 35

[Collin Jackson]

Comment on attachment 296463 [details] [diff] [review]
Updated patch with mochitests

Jesse, are you available to review this?

Comment 36

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the changes will need review from one of bsmebdberg, jst, or me (or darin, but he's not really active) per <http://www.mozilla.org/owners.html>.  This is apart from the security review, which should be dveditz or Jesse.

I do have this on my list of things to review, but likely won't get to it until next weekend.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 296463 [details] [diff] [review]
Updated patch with mochitests

jst, dveditz said that you'll be a better reviewer, here

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 296463 [details] [diff] [review]
Updated patch with mochitests

Boris, any chance you could glance over the docshell changes here too? They're not big changes (the majority of the patch is tests), so shouldn't take you long...

Comment 39

[Boris Zbarsky [:bzbarsky]]

The tests are the part that needs review (and comparison to the old bugs in this code)...  I'm still hoping to get to this before the freeze.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 296463 [details] [diff] [review]
Updated patch with mochitests

>Index: docshell/base/nsDocShell.cpp
>+    //  2) aTargetItem is a top-level frame and we are its descendant or can
>+    //     access its opener.

"we" should be "aAccessingItem", right?

Note that this doesn't quite describe what you implemented.  What's implemented is more like:

  2) aTargetItem is a top-level frame and we are its descendant
  3) aTargetItem is a top-level frame and we can target its opener
     per rule (1) or (2).

I guess we can do either one, but I would be fine with the more restrictive version described in your comment, I think.  In particular, that would prevent off-site frames targeting on-site popups opened from toplevel, which is something I do think we want to prevent (and test for).

>-        if (ValidateOrigin(aAccessingItem, target)) {
>+        if (ValidateOrigin(aAccessingItem, target))
>             return PR_TRUE;
>-        }

Please leave the curlies there?  Similar for the other one-line if bodies in this patch.

>Index: docshell/test/Makefile.in

> DIRS += chrome \
> 	browser \
>+	navigation \

Why not just put the new files directly in docshell/test?  It probably doesn't matter all that much, but just wondering...

>Index: docshell/test/navigation/NavigationUtils.js
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation

Just making sure that this is correct... if you guys just wrote this on your own without the Foundation paying you for it, chances are you're the Initial Developers.  Again, not a big deal either way, just making sure.

>+function navigateByLocation(wnd) {

>+    window.open(target_url, "_blank");

Could you toss on a "width=10,height=10" feature string on there?  That helps with running the tests when the default for new DOMWindows is new window, not new tabs (which is something we do need to test every so often).  Similar for the other window.open calls in the tests.

>+function isNavigated(wnd, message) {

>+  } catch(ex) {
>+    result = ex;

I assume that you want to press on here on purpose (e.g. because you want to clean up the windows) instead of just relying on the exception to trigger a mochitest failure?  Similar in isBlank, isAccessible, isInaccessible.

>+function xpcGetFramesByName(name) {

>+  var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                     .getService(Components.interfaces.nsIWindowMediator);

Is there a reason to not use nsIWindowWatcher's window enumerator here instead?  That would be a little more robust, I think, esp. in terms of not relying on the exact structure of the Firefox chrome.  Or did you try it and it failed for some reason?  This applies to all the xpc* functions.

This is not recursing into subframes on purpose, I assume?  Please document that (and perhaps the reason why it doesn't descend if there is a short explanation past "that's what the test expects").

>Index: docshell/test/navigation/test_bug270414.html

It's good to have this test (and we should keep it), but this doesn't test bug 270414.  To test that bug, the window in which the navigation originates needs to be generated completely via document.write().

>Index: docshell/test/navigation/test_bug278143.html

This looks like it tests bug 279495, but not so much bug 278143.  Testing bug 278143 might be kind of tough now that I look at it, and shouldn't be affected much by this change, so we can just let it be.  Just rename this test so it's clear what it's doing?

>+function countAndClose(name, expected_count) {
>+  is(array_of_frames.length, expected_count, "Should only open " + expected_count + " window using a fancy hyperlink.");

Maybe

   ... "window with name " + name + " using ....

?

>Index: docshell/test/navigation/test_bug278916.html

This test will definitely need to use windowwatcher to be meaningful, since bug 278916 only showed up in Gecko embeddings that don't have a window mediator...

This looks really really good in general.  Thank you very much for writing up the tests; they made it a lot easier to both review the patch and convince myself that the patch is correct!

If you want to land the tests without doing the window mediator to window watcher switch and do that switch as a followup bug, that would be fine by me.  That's not an issue that should hold up landing.  Same for the bug270414 issue.  I'd appreciate resolving the other test comments before checkin; they're pretty minor.

And most importantly, let's sort out the (2) thing from nsDocShell.cpp?

Comment 41

[Adam Barth]

Attachment: Updated patch addressing reviewer comments

> "we" should be "aAccessingItem", right?

Yep.  Fixed.

> Note that this doesn't quite describe what you implemented.  What's
> implemented is more like:
>
>  2) aTargetItem is a top-level frame and we are its descendant
>  3) aTargetItem is a top-level frame and we can target its opener
>     per rule (1) or (2).

Yep.  This comment was sloppily written.  Fixed.

> Please leave the curlies there?

Fixed for the C++ code.

> Why not just put the new files directly in docshell/test?  It probably
> doesn't matter all that much, but just wondering...

We left these in their own directory.  The tests have a bunch of auxiliary files that only seem useful in navigation tests.

> chances are you're the Initial Developers.

Ok.  Updated to say "Stanford University" since it writes our paychecks.

> Could you toss on a "width=10,height=10" feature string on there?

Done.  Not all the windows are opened via window.open, but the ones that are now all open in small popup windows.

> I assume that you want to press on here on purpose

Yes.  We don't want these APIs to throw exceptions.  In fact, some times the exceptional behavior is correct (e.g. in isInaccessible).

> Is there a reason to not use nsIWindowWatcher's window enumerator
> here instead?

Changed to using nsIWindowWatcher's enumerator, but that seems to still give us back a chrome window (so we have to reach inside to find the content window).  The getWindowByName isn't very helpful because we need to enumerate multiple frames with the same name, for example when a navigation fails and spawns a new window.

> This is not recursing into subframes on purpose, I assume?

Yes, we're just looking for top-level frames here.  We could recurse here, but it doesn't affect the test.

> Please document that

Fixed.

> It's good to have this test (and we should keep it), but this doesn't test 
> bug 270414.  To test that bug, the window in which the navigation originates
> needs to be generated completely via document.write().

Fixed.  The old test has been renamed test_popup_navigates_children.html

> This looks like it tests bug 279495, but not so much bug 278143.
> [...]
> Just rename this test so it's clear what it's doing?

Done.

> Maybe   ... "window with name " + name + " using .... ?

Done.

> This test will definitely need to use windowwatcher to be meaningful, 
> since bug 278916 only showed up in Gecko embeddings that don't have a
> window mediator...

Ok.  We're using window watcher now.

> If you want to land the tests without doing the window mediator to window
> watcher switch and do that switch as a followup bug, that would be fine by
> me. That's not an issue that should hold up landing.  Same for the
> bug270414 issue. I'd appreciate resolving the other test comments before
> checkin; they're pretty minor.

We've tried to resolve all the issues you brought up.  I'm not sure we're using WindowWatcher in the way you intend (we still seem to be relying on the structure of chrome windows), but maybe that's an issue to address in a separate patch.

> And most importantly, let's sort out the (2) thing from nsDocShell.cpp?

We left this as coded to most-closely match current Firefox behavior.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 299478 [details] [diff] [review]
Updated patch addressing reviewer comments

> Changed to using nsIWindowWatcher's enumerator, but that seems to still give
> us back a chrome window

Ugh.  I forgot just how messed-up this stuff is...  So these tests still won't work in embedding builds, I guess, unless we add a branch for the case when win.gBrowser is undefined (in which case, we want to look at win.location instead of digging for tabs).  Perhaps we should do that, in fact:

  if (win.gBrowser) {
    // The code you have now
  } else if (win.location.protocol == "data:") {
    // Embedding case
    win.close();
  }

with similar changes to the other window watcher consumers here.

That should probably be a separate patch, since I'd like to get this change in soon so we can get a bit of bake before the beta.

r=bzbarsky.  Thanks again for doing this!

Comment 43

[Boris Zbarsky [:bzbarsky]]

I've checked the patch in on trunk.  Adam, can you get a followup filed for the minor windowwatcher tweak?

As written, the patch caused build bustage; to fix that I removed the "XPCSHELL_TESTS = unit" line from the makefile in docshell/test/navigation

This patch also caused a test failure for test_bug326337.html.  I fixed that by moving the test code around so that the location is being set on a toplevel window, not a subframe of a toplevel window.

Comment 44

[Collin Jackson]

I filed bug 414303 regarding the nsIWindowWatcher issue.

Comment 45

[Steve England [:stevee]]

It looks like this caused bug 418559.

Comment 46

[Daniel Veditz [:dveditz]]

Even though this is sort of a web-compat change it seems like it would still be good to land on the 1.8 branch so we get consistent behavior. There are currently significant numbers of browsers in common use with this behavior (IE7) and as FF3 releases will soon be tons more.

Comment 47

[Daniel Veditz [:dveditz]]

Given the risk of site regressions we'd like to wait for the Firefox 3 release to see how this goes over before changing the branch behavior.