Closed Bug 860941 Opened 7 years ago Closed 7 years ago

Clean up showModalDialog implementation

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(10 files)

4.22 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.03 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.50 KB, patch
jst
: review+
Details | Diff | Splinter Review
1020 bytes, patch
jst
: review+
Details | Diff | Splinter Review
26.07 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.79 KB, patch
jst
: review+
Details | Diff | Splinter Review
6.96 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.50 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.66 KB, patch
jst
: review+
Details | Diff | Splinter Review
14.89 KB, patch
jst
: review+
Details | Diff | Splinter Review
the arguments handling between nsGlobalWindow and nsWindowWatcher is pretty nutty, and has bitten me on multiple occasions. Furthermore, it currently causes reproducible asserts anytime someone creates a modal dialog, which we've had to annotate in the test suite.

I'm going to see how much I can do to improve the situation here.
Blocks: 841312
Depends on: 862918
Blocks: 600703
Blocks: 829310
This has...expanded in scope.
Summary: Clean up dialog arguments handling → Clean up showModalDialog implementation
I'm not sure what it used to do, but it sure doesn't do a damn thing now.
Attachment #748112 - Flags: review?(jst)
Tracking this with CHROME_MODAL is problematic, because that gets inherited by
any dependent windows opened by the modal content window, which may or may not
be modal content windows themselves. Thankfully, we have a few free bits lying
around.
Attachment #748113 - Flags: review?(jst)
While the mArguments invariant should hold for _outers_, it doesn't necessarily
hold for inners, so this assertion fires reliably in automation. If mCleanedUp
is true then mArguments is definitely null, so let's disentangle this from
mArguments and be clearer about the invariants we expect.
Attachment #748115 - Flags: review?(jst)
This function proceeds to invoke CleanUp(), which also cleans this stuff up.
Attachment #748116 - Flags: review?(jst)
This patch is bigger than I'd like it to be, but there are a lot of interlocked
dependencies and I eventually decided it was easier to just lump it together.

The semantics of |showModalDialog|/|window.dialogArguments| (an web-exposed
HTML5 feature) and |openDialog|/|window.arguments| (a XUL-proprietary feature)
are quite different. The former is essentially a security-checked JSVal, while
the latter gets converted into an array. We handled them together in the old
world, which led to a lot of confusion and muddled semantics. This patch
separates them.

This patch also eschews the roundabout resolve hook for dialogArguments in favor
of returning them directly from the XPIDL getter. This better matches the
behavior in the spec, especially because it allows dialogArguments to live on
the outer as they're supposed to, rather than the first inner that happens to
end up in the docshell. All in all, this should make this all very
straightforward to convert WebIDL when the time comes.

The current spec on the origin checks here is pretty fictional, so I've filed
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21932 to fix it. This patch
should more or less preserve the current security behavior.
Attachment #748117 - Flags: review?(jst)
This is correct by my reading of the spec. Quoting:

The dialogArguments IDL attribute, on getting, must check whether its browsing
context's active document's origin is the same as the dialog arguments' origin.
If it is, then the browsing context's dialog arguments must be returned
unchanged. Otherwise, if the dialog arguments are an object, then the empty
string must be returned, and if the dialog arguments are not an object, then
the stringification of the dialog arguments must be returned.
Attachment #748119 - Flags: review?(jst)
The spec currently has returnValue as a DOMString, but this doesn't match
reality given my testing. I filed [1] to fix it.

Note that nsGlobalModalWindow is already set up to CC mReturnValue. Since
we're swapping in another CC-ed container class, we don't need to make any
changes here.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=21771
Attachment #748120 - Flags: review?(jst)
Since this stuff is a property on the browsing context, this only makes sense
as a security check. But now that we're using a DialogValueHolder, the origin
checks are taken care of. So we can kill this off.
Attachment #748121 - Flags: review?(jst)
We augment the existing showModalDialog tests with test coverage for
dialogArguments and returnValue.
Attachment #748122 - Flags: review?(jst)
Comment on attachment 748117 [details] [diff] [review]
Part 5 - Separate the handling of |dialogArguments| and |arguments|, and use IDL for the |dialogArguments| getter. v2

Review of attachment 748117 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.h
@@ +233,5 @@
>      MOZ_COUNT_DTOR(IdleObserverHolder);
>    }
>  };
>  
> +inline already_AddRefed<nsIVariant>

static

@@ +236,5 @@
>  
> +inline already_AddRefed<nsIVariant>
> +CreateVoidVariant()
> +{
> +    nsCOMPtr<nsIWritableVariant> writable =

2-space

@@ +260,5 @@
> +
> +  DialogValueHolder(nsIPrincipal *aSubject, nsIVariant *aValue)
> +    : mOrigin(aSubject)
> +    , mValue(aValue) {}
> +  nsresult Get(nsIPrincipal *aSubject, nsIVariant **aResult) {

* to the left; { on the next line

::: dom/base/nsJSEnvironment.cpp
@@ +1685,4 @@
>    }
>  
> +  JSObject *args = ::JS_NewArrayObject(mContext, argc, argv);
> +  vargs = OBJECT_TO_JSVAL(args);

Move the declaration of vargs down here.

::: dom/interfaces/base/nsIDOMModalContentWindow.idl
@@ +13,5 @@
>  {
>    /**
> +   * Readonly attribute containing an arbitrary JS value passed by the
> +   * code that opened the modal content window. A security check is
> +   * performed at access time, per spec.

What spec?
Comment on attachment 748115 [details] [diff] [review]
Part 3 - Clarify shutdown invariants in ~nsGlobalWindow. v3

Review of attachment 748115 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +1239,5 @@
> +  // separating the handling into CleanUp() and FreeInnerObjects.
> +  if (IsInnerWindow())
> +    CleanUp(true);
> +  else
> +    MOZ_ASSERT(mCleanedUp);

{}. Can you assert that CleanUp is only called on outers now?
Attachment #748112 - Flags: review?(jst) → review+
Attachment #748113 - Flags: review?(jst) → review+
Attachment #748115 - Flags: review?(jst) → review+
Attachment #748116 - Flags: review?(jst) → review+
Comment on attachment 748117 [details] [diff] [review]
Part 5 - Separate the handling of |dialogArguments| and |arguments|, and use IDL for the |dialogArguments| getter. v2

Looks good, and lots of good cleanup here. r=jst with one nit.

+inline already_AddRefed<nsIVariant>
+CreateVoidVariant()
+{
+    nsCOMPtr<nsIWritableVariant> writable =
+      do_CreateInstance(NS_VARIANT_CONTRACTID);

2-space indentation here please.
Attachment #748117 - Flags: review?(jst) → review+
Attachment #748119 - Flags: review?(jst) → review+
Comment on attachment 748120 [details] [diff] [review]
Part 7 - Use DialogValueHolder for returnValue. v1

># HG changeset patch
># User Bobby Holley <bobbyholley@gmail.com>
>
>Bug 860941 - Use DialogValueHolder for returnValue. v1
>
>The spec currently has returnValue as a DOMString, but this doesn't match
>reality given my testing. I filed [1] to fix it.
>
>Note that nsGlobalModalWindow is already set up to CC mReturnValue. Since
>we're swapping in another CC-ed container class, we don't need to make any
>changes here.
>
>[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=21771
>
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index 2e38d10..6357979 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -7693,67 +7693,29 @@ nsGlobalWindow::ShowModalDialog(const nsAString& aURI, nsIVariant *aArgs_,
>                              true,           // aDoJSFixups
>                              true,           // aNavigate
>                              nullptr, argHolder, // args
>                              GetPrincipal(),     // aCalleePrincipal
>                              nullptr,            // aJSCallerContext
>                              getter_AddRefs(dlgWin));
>   nsContentUtils::SetMicroTaskLevel(oldMicroTaskLevel);
>   LeaveModalState(callerWin);
>-
>   NS_ENSURE_SUCCESS(rv, rv);
>-  
>-  if (dlgWin) {
>-    nsCOMPtr<nsIPrincipal> subjectPrincipal;
>-    rv = nsContentUtils::GetSecurityManager()->
>-      GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>-    if (NS_FAILED(rv)) {
>-      return rv;
>-    }
> 
>-    bool canAccess = true;
>-
>-    if (subjectPrincipal) {
>-      nsCOMPtr<nsIScriptObjectPrincipal> objPrincipal =
>-        do_QueryInterface(dlgWin);
>-      nsCOMPtr<nsIPrincipal> dialogPrincipal;
>-
>-      if (objPrincipal) {
>-        dialogPrincipal = objPrincipal->GetPrincipal();
>-
>-        rv = subjectPrincipal->Subsumes(dialogPrincipal, &canAccess);
>-        NS_ENSURE_SUCCESS(rv, rv);
>-      } else {
>-        // Uh, not sure what kind of dialog this is. Prevent access to
>-        // be on the safe side...
>-
>-        canAccess = false;
>-      }
>-    }
>-
>-    nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(dlgWin));
>-
>-    if (canAccess) {
>-      nsPIDOMWindow *inner = win->GetCurrentInnerWindow();
>-
>-      nsCOMPtr<nsIDOMModalContentWindow> dlgInner(do_QueryInterface(inner));
>-
>-      if (dlgInner) {
>-        dlgInner->GetReturnValue(aRetVal);
>-      }
>-    }
>-
>-    nsRefPtr<nsGlobalWindow> winInternal =
>-      static_cast<nsGlobalWindow*>(win.get());
>-    if (winInternal->mCallCleanUpAfterModalDialogCloses) {
>-      winInternal->mCallCleanUpAfterModalDialogCloses = false;
>-      winInternal->CleanUp(true);
>+  nsCOMPtr<nsIDOMModalContentWindow> dialog = do_QueryInterface(dlgWin);
>+  if (dialog) {
>+    rv = dialog->GetReturnValue(aRetVal);
>+    MOZ_ASSERT(NS_SUCCEEDED(rv));
>+    nsGlobalModalWindow *win = static_cast<nsGlobalModalWindow*>(dialog.get());
>+    if (win->mCallCleanUpAfterModalDialogCloses) {
>+      win->mCallCleanUpAfterModalDialogCloses = false;
>+      win->CleanUp(true);
>     }
>   }
>-  
>+
>   return NS_OK;
> }
> 
> class CommandDispatcher : public nsRunnable
> {
> public:
>   CommandDispatcher(nsIDOMXULCommandDispatcher* aDispatcher,
>                     const nsAString& aAction)
>@@ -11659,28 +11621,32 @@ nsGlobalModalWindow::GetDialogArguments(nsIVariant **aArguments)
>   return mDialogArguments->Get(nsContentUtils::GetSubjectPrincipal(), aArguments);
> }
> 
> NS_IMETHODIMP
> nsGlobalModalWindow::GetReturnValue(nsIVariant **aRetVal)
> {
>   FORWARD_TO_OUTER_MODAL_CONTENT_WINDOW(GetReturnValue, (aRetVal), NS_OK);
> 
>-  NS_IF_ADDREF(*aRetVal = mReturnValue);
>-
>-  return NS_OK;
>+  nsCOMPtr<nsIVariant> result;
>+  if (!mReturnValue) {
>+    nsCOMPtr<nsIVariant> variant = CreateVoidVariant();
>+    variant.forget(aRetVal);
>+    return NS_OK;
>+  }
>+  return mReturnValue->Get(nsContentUtils::GetSubjectPrincipal(), aRetVal);
> }
> 
> NS_IMETHODIMP
> nsGlobalModalWindow::SetReturnValue(nsIVariant *aRetVal)
> {
>   FORWARD_TO_OUTER_MODAL_CONTENT_WINDOW(SetReturnValue, (aRetVal), NS_OK);
> 
>-  mReturnValue = aRetVal;
>-
>+  mReturnValue = new DialogValueHolder(nsContentUtils::GetSubjectPrincipal(),
>+                                       aRetVal);
>   return NS_OK;
> }
> 
> nsresult
> nsGlobalModalWindow::SetNewDocument(nsIDocument *aDocument,
>                                     nsISupports *aState,
>                                     bool aForceReuseInnerWindow)
> {
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>index e9cfb72..d4cf9e6 100644
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -247,16 +247,19 @@ CreateVoidVariant()
> //
> // Given our clunky embedding APIs, modal dialog arguments need to be passed
> // as an nsISupports parameter to WindowWatcher, get stuck inside an array of
> // length 1, and then passed back to the newly-created dialog.
> //
> // However, we need to track both the caller-passed value as well as the
> // caller's, so that we can do an origin check (even for primitives) when the
> // value is accessed. This class encapsulates that magic.
>+//
>+// We also use the same machinery for |returnValue|, which needs similar origin
>+// checks.
> class DialogValueHolder : public nsISupports
> {
> public:
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_CLASS(DialogValueHolder)
> 
>   DialogValueHolder(nsIPrincipal *aSubject, nsIVariant *aValue)
>     : mOrigin(aSubject)
>@@ -1327,17 +1330,18 @@ public:
> 
>   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsGlobalModalWindow, nsGlobalWindow)
> 
>   virtual NS_HIDDEN_(nsresult) SetNewDocument(nsIDocument *aDocument,
>                                               nsISupports *aState,
>                                               bool aForceReuseInnerWindow);
> 
> protected:
>-  nsCOMPtr<nsIVariant> mReturnValue;
>+  // For use by outer windows only.
>+  nsRefPtr<DialogValueHolder> mReturnValue;
> };
> 
> /* factory function */
> inline already_AddRefed<nsGlobalWindow>
> NS_NewScriptGlobalObject(bool aIsChrome, bool aIsModalContentWindow)
> {
>   nsRefPtr<nsGlobalWindow> global;
> 
>diff --git a/dom/tests/mochitest/bugs/test_bug437361.html b/dom/tests/mochitest/bugs/test_bug437361.html
>index bc2b002..6db31c3 100644
>--- a/dom/tests/mochitest/bugs/test_bug437361.html
>+++ b/dom/tests/mochitest/bugs/test_bug437361.html
>@@ -15,17 +15,17 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=437361
>   }
> 
>   /** Test for Bug 437361 **/
> 
>   function testModalDialogBlockedCleanly() {
>     is(true, SpecialPowers.getBoolPref("dom.disable_open_during_load"), "mozprefs sanity check");
>     var rv = window.showModalDialog( // should be blocked without exception
>       "data:text/html,<html><body onload='close(); returnValue = 1;' /></html>");
>-    is(rv, null, "Modal dialog opened unexpectedly.");
>+    is(rv, undefined, "Modal dialog opened unexpectedly.");
>   }
>   
>   function testModalDialogAllowed() {
>     is(false, SpecialPowers.getBoolPref("dom.disable_open_during_load"), "mozprefs sanity check");
>     var rv = window.showModalDialog( // should not be blocked this time
>       "data:text/html,<html><body onload='close(); returnValue = 1;' /></html>");
>     is(rv, 1, "Problem with modal dialog returnValue.");
>   }
Attachment #748120 - Flags: review?(jst) → review+
Attachment #748121 - Flags: review?(jst) → review+
Attachment #748122 - Flags: review?(jst) → review+
Attachment #748123 - Flags: review?(jst) → review+
Hmm, sorry for including the whole patch in comment 18, unclear how that happened :(
(In reply to :Ms2ger from comment #15)
> >    /**
> > +   * Readonly attribute containing an arbitrary JS value passed by the
> > +   * code that opened the modal content window. A security check is
> > +   * performed at access time, per spec.
> 
> What spec?

HTML5.

(In reply to :Ms2ger from comment #16)
> Can you assert that CleanUp is only called on outers now?

Not trivially. See the comment.
Arg, I made some last-minute changes to the test which broke when the test runs in a frame (as it does in the harness), but not when the test runs standalone (which is what I tested). Fixed, and repushed:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c98f7f0305a7
Depends on: 933338
No longer depends on: 933338
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.