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.

This has...expanded in scope.

I'm not sure what it used to do, but it sure doesn't do a damn thing now.

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.

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.

This function proceeds to invoke CleanUp(), which also cleans this stuff up.

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.

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.

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

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.

We augment the existing showModalDialog tests with test coverage for dialogArguments and returnValue.

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?

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.

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."); > }

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.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/70c3a3a74362 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c47a46c92dff remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/98594535c1e9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/474792d1fb89 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad1a92ca568 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/90b318fb8375 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/95006bb32743 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/de30d8faf09a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2505c0f1c45 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/34c65a615373

Unfortunately this had to be backed out for mochitest-3 timeouts in test_showModalDialog.html: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=34c65a615373 https://tbpl.mozilla.org/php/getParsedLog.php?id=23069915&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=23069925&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb5afba9988 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e55f3b87bdf5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/87cf098ea38b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee6de720fef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5769c624b7b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba756b1cbde9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b57c879e73a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/67944b9cf98e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c41a477c2dd8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e70420bd6cde

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

https://hg.mozilla.org/mozilla-central/rev/7bcc1ec6fa87 https://hg.mozilla.org/mozilla-central/rev/76ab368d6b55 https://hg.mozilla.org/mozilla-central/rev/6f1c775378c3 https://hg.mozilla.org/mozilla-central/rev/688bd6e0dc3a https://hg.mozilla.org/mozilla-central/rev/e4c145a09a94 https://hg.mozilla.org/mozilla-central/rev/af1862248694 https://hg.mozilla.org/mozilla-central/rev/8634d3597ee8 https://hg.mozilla.org/mozilla-central/rev/6bd149e8edfc https://hg.mozilla.org/mozilla-central/rev/017883faccbb https://hg.mozilla.org/mozilla-central/rev/341f9653ff92