Re-factor nsWindowWatcher::OpenWindowInternal to not be such a rat's nest

RESOLVED INCOMPLETE

Status

()

Core
General
RESOLVED INCOMPLETE
2 years ago
2 years ago

People

(Reporter: mconley, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 obsolete attachments)

nsWindowWatcher::OpenWindowInternal is what eventually gets called when a new window or tab needs to be opened.

This method... does a lot. A _lot_. And it's used in both the content process and the parent process, and there is lots of special-casing throughout the method for the type of window that's being opened, the privileges of the caller, the URL we're going to, which process we're in... a lot of stuff.

It makes reasoning about window opening very, very difficult. The code is brittle, and that makes it really hard to change with any degree of confidence that you haven't caused some subtle bugs somewhere. As there are various security protections set up in OpenWindowInternal, that can get pretty dice-y.

Bug 1261842 is, at least for me, the straw that is going to break the back of the camel here. In that bug, I need to set the size of the newly opened window in the parent process, but with the added complication that the primary content shell will no longer exist. This worries me because OpenWindowInternal falls back to using the docShell of the chrome window itself then to make various decisions and set various properties instead of on the docshell of the newly opened window, and that worries me a great deal.

So I want to refactor this mutha of a method. I want to break it into smaller bits that are well defined, and make it clearer under what conditions we want to use certain bits.

Wish me luck.
Having studied OpenWindowInternal for a day or two now, I think I can break it down into a few phases. There's some bleed-over from phase to phase, but I think it can be safely reorganized to partition them from one another.

Here's the outcome I'm aiming for:

nsWindowWatcher::OpenWindowInternal(args1, args2, args3, argsforever)
{
  // Common preparation
  // Create Trait based on arguments. Trait does assertions and internal preparation.

  // Ask Trait to find or create us a window, returning a bool for whether or not we’re re-using a window, and a result code.
  // Ensure that we have a window by now, and return an error code if we do not.
  // Do common security checks, assertions, and setup
  // Do Trait-specific security checks, assertions and setup

  // If we created a new window, do common new window configuration
  // If we created a new window, do Trait-specific new window configuration

  // Do common post-configuration tasks
  // Do Trait-specific post-configuration tasks
}

Where a Trait will be a C-struct, specializing in one of the following cases:

1) We're in the content process
2) We're in the parent process, responding to a request from the content process to open a new window
3) We're in the parent process, and we're responding to a request to open a new window from anything except the content process
Since in some cases, there might be some overlap between 2 of the Traits (but not 3, since that'd make me put it in "common"), I'll probably break that code out into helper functions that can be called from the Trait methods.
A top-to-bottom review of this code has shown that there's really not much that's common between all three. There's plenty of overlap between the (1) and (3) case of the cases I listed in comment 1, and a little bit of overlap between (2) and (3). I don't believe there's any overlap between (1) and (2).

That's going to inform my refactoring somewhat. Stay tuned.
Created attachment 8748869 [details]
MozReview Request: Bug 1267720 - Tag a bunch of tests that exercise opening windows with openwindow. r=me

Review commit: https://reviewboard.mozilla.org/r/50563/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50563/
Created attachment 8748870 [details]
MozReview Request: Bug 1267720 - Test that new windows opened from remote content get the right flags. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50565/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50565/
Created attachment 8748873 [details]
MozReview Request: Bug 1267720 - Add a test for chromeflags for new windows from content. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50571/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50571/
Created attachment 8748874 [details]
MozReview Request: Bug 1267720 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows. r?ehsan

Review commit: https://reviewboard.mozilla.org/r/50573/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50573/
Created attachment 8748875 [details]
MozReview Request: Bug 1267720 - Add a test for the size of newly opened window from content. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50575/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50575/
Created attachment 8748876 [details]
MozReview Request: Bug 1267720 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r?anybody

Review commit: https://reviewboard.mozilla.org/r/50577/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50577/
Created attachment 8748877 [details]
MozReview Request: Bug 1267720 - Add a test to ensure that we clone sessionStorage when opening new windows. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50579/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50579/
Created attachment 8748878 [details]
MozReview Request: Bug 1267720 - Test that modal windows can be opened from the parent process. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50581/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50581/
Created attachment 8748879 [details]
MozReview Request: Bug 1267720 - Test that newly opened dialogs can receive arguments. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50583/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50583/
Created attachment 8748881 [details]
MozReview Request: Bug 1267720 - Make parent outer window available on TabParent. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50587/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50587/
Created attachment 8748882 [details]
MozReview Request: Bug 1267720 - Stop sending the window name to ContentParent when opening a new window. r?smaug

By the time that the parent is being asked to create a new window, the name
really doesn't matter anymore.

Review commit: https://reviewboard.mozilla.org/r/50589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50589/
Created attachment 8748883 [details]
MozReview Request: Bug 1267720 - Make initial browser remote sooner if we're defaulting to using remote tabs. r?felipe

Review commit: https://reviewboard.mozilla.org/r/50591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50591/
Created attachment 8748884 [details]
MozReview Request: Bug 1267720 - Factor out logic for creating windows for content processes from OpenWindowInternal

Review commit: https://reviewboard.mozilla.org/r/50593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50593/
Just checkpointing - there's still a bit of work here.
The work for this was done in bug 1261842. The logic for opening windows from child processes was extracted from OpenWindowInternal, and specialized/isolated logic for processing chromeFlags for windows opened by content were added.

I don't think there's any reason to keep this bug open.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.