Justin Dolske [:Dolske] Reporter
	Description • 14 years ago

When modal prompts open/close, they fire DOMWillOpenModalDialog and DOMModalDialogClosed events at the window. These events are visible to content for no good reason. We should just change them to be notifications, and have tabbrowser observe them instead of listen for events.

See nsPrompter.js / fireEvent() and nsAutoWindowStateHelper, where these events are fired.

	Frank Yan (:fryn) Assignee
	Comment 1 • 13 years ago

This patch is pretty much complete.
I noticed that it seems to be firing two notifications (instead of one) for window.showModalDialog, but I'm not sure what the cause is. Help debugging that would be appreciated.

I got "mozilla::services is not defined" errors until I #include'd "mozilla/Services.h", but I don't know if that is overkill. Is there a more efficient way?

Tim, some feedback on the tabview/ bits would be much appreciated. :)

	Frank Yan (:fryn) Assignee
	Comment 2 • 13 years ago

Instead of using Array.some(...), here's a simpler onDOMWillOpenModalDialog(cx) in ui.js:

  _onDOMWillOpenModalDialog: function UI__onDOMWillOpenModalDialog(cx) {
    if (!this.isTabViewVisible())
      return;

    // When TabView is visible, we need to call onTabSelect to make sure that
    // TabView is hidden and that the correct group is activated. When a modal
    // dialog is shown for currently selected tab the onTabSelect event handler
    // is not called, so we need to do it.
    let tab = gBrowser._getTabForContentWindow(cx.top);
    if (gBrowser.selectedTab == tab && this._currentTab == tab)
      this.onTabSelect(tab);
  },

	Tim Taubert [:ttaubert] (inactive)
	Comment 3 • 13 years ago

Comment on attachment 582168 [details] [diff] [review]
patch

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

I only looked at the Panorama-specific code. F+ if you just remove all the modal dialog code in tabview/content.js :)

::: browser/components/tabview/content.js
@@ +84,4 @@
>  
> +// add observer
> +Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService).
> +addObserver(ModalDialogObserver, "dom-will-open-modal-dialog", false);

Since that's a global observer notification we don't need to handle this in a content script anymore. Even worse, this observer is added once for every opened tab (and handled although there's no code to check the sender).

::: browser/components/tabview/ui.js
@@ +254,2 @@
>        let callback = this._onDOMWillOpenModalDialog.bind(this);
> +      Services.obs.addObserver(callback, "dom-will-open-modal-dialog", false);

You're removing the message listener here so you could just remove all the modal dialog code from content.js (we don't need that anymore).

@@ +978,5 @@
>  
> +    let index = -1;
> +    let contentWin = cx.top;
> +    Array.some(gBrowser.browsers, function(browser, i) {
> +      if (browser.contentWindow == contentWin) {

This works but isn't e10s-proof at all. I'm not sure what's the current guide line regarding stuff that breaks in e10s... Otherwise that's ok.

I don't think we should use gBrowser._getTabForContentWindow() here (although that looks much cleaner) and access the browser's internal API. We should rather re-implement it (as you've done) or make the API 'public'.

	Frank Yan (:fryn) Assignee
	Comment 4 • 13 years ago

(In reply to Tim Taubert [:ttaubert] from comment #3)
> I only looked at the Panorama-specific code. F+ if you just remove all the
> modal dialog code in tabview/content.js :)

Ah, right; I accidentally left some in. :P

> I'm not sure what's the current
> guide line regarding stuff that breaks in e10s... Otherwise that's ok.

IIRC, we aren't actively working on e10s anymore, and we don't need to ensure that our code is e10s-safe.

> I don't think we should use gBrowser._getTabForContentWindow() here
> (although that looks much cleaner) and access the browser's internal API. We
> should rather re-implement it (as you've done) or make the API 'public'.

I don't think it makes much sense to re-implement the internal API given that browser/components/tabview/ is part of our own code, and there is no _real_ distinction between what is public and private when working with our code.
Re-implementing it just makes it more likely for our code to become out-of-sync with itself.

Thanks for the feedback. :)

	Justin Dolske [:Dolske] Reporter
	Comment 5 • 13 years ago

Could you do a quick test to see if these events are being fired in IE/Chrome/Safari/Opera? I'm mostly sure they're not, but just want to ensure we're actually removing a non-standard thing.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 6 • 13 years ago

Wouldn't it be simpler to switch this to an event that's only propagated to the chrome event handler? There are easy ways to do that, IIRC - smaug knows the details. That way you avoid the overhead running the handler code in every window (and the need for same-window checks, some of which seem to be missing in the mobile portions of that patch).

	Frank Yan (:fryn) Assignee
	Comment 7 • 13 years ago

(In reply to Justin Dolske [:Dolske] from comment #5)
> Could you do a quick test to see if these events are being fired in
> IE/Chrome/Safari/Opera? I'm mostly sure they're not, but just want to ensure
> we're actually removing a non-standard thing.

The events are not being fired in IE, Chrome, Safari, and Opera.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> Wouldn't it be simpler to switch this to an event that's only propagated to
> the chrome event handler? There are easy ways to do that, IIRC - smaug knows
> the details. That way you avoid the overhead running the handler code in
> every window (and the need for same-window checks, some of which seem to be
> missing in the mobile portions of that patch).

I'm happy to do whichever is better. What do you think, Dolske?

	Justin Dolske [:Dolske] Reporter
	Comment 8 • 13 years ago

My primary concern was exposure to content, either approach should be fine.

Though the converse of gavin's point in comment 6 is that is something (an addon, say) wants to be aware of dialogs, it has to go through extra work to track windows being opened and add listeners to them. :)

Hrm, now that I think about it, I was considering moving password manager to such a model (ie, instead of implementing nsIPromptFu itself, it would just watch for prompts being opened/closed and fill/save as needed). But I've no immediate plans to do that, and it's only tangentially relevant here.

So, yeah, "whatever". :)

	Justin Dolske [:Dolske] Reporter
	Comment 9 • 13 years ago

Comment on attachment 582168 [details] [diff] [review]
patch

(Clearing feedback-? on the assumption a new patch will be quite different, if you want me to look at the current one just flag me again.)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 12 years ago

Comment on attachment 702580 [details] [diff] [review]
patch


>-nsAutoWindowStateHelper::DispatchCustomEvent(const char *aEventName)
>+nsAutoWindowStateHelper::DispatchChromeEvent(const char *aEventName)
Could you call this DispatchEventToChrome

>+  window->GetExtantDocument()->CreateEvent(NS_LITERAL_STRING("Events"),
>+                                           getter_AddRefs(event));
Null check GetExtandDocument()

>+  event->InitEvent(NS_ConvertASCIItoUTF16(aEventName), true, true);
The return value of InitEvent should be checked.
NS_ENSURE_TRUE(NS_SUCCEEDED(event->InitEvent(NS_ConvertASCIItoUTF16(aEventName), true, true)), false);

	Justin Dolske [:Dolske] Reporter
	Comment 12 • 12 years ago

Comment on attachment 702580 [details] [diff] [review]
patch

Oorah!

	Frank Yan (:fryn) Assignee
	Comment 13 • 12 years ago

Patching PromptService.js in /mobile/android/.
It depends on the other patch.

	Frank Yan (:fryn) Assignee
	Comment 14 • 12 years ago

Oops, uploaded the wrong file. Sorry.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 12 years ago

Some test would be really nice.

	Frank Yan (:fryn) Assignee
	Comment 16 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/64eb6292b965
https://hg.mozilla.org/integration/mozilla-inbound/rev/9229b7917f47

(In reply to Olli Pettay [:smaug] from comment #15)
> Some test would be really nice.

Okay. I'll try to get to it.
I suppose I could structure it as a chrome-privileged test that adds an event listener and also generates a web page that adds an identical event listener and then displays a prompt.
Thanks for the review!

	Ed Morley [:emorley]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/64eb6292b965
https://hg.mozilla.org/mozilla-central/rev/9229b7917f47

	Frank Yan (:fryn) Assignee
	Comment 18 • 12 years ago

The risk here is low.
We didn't document this event to begin with, and if any web page is using it, it shouldn't be, since it can detect its own calls to create prompts (alert, prompt, confirm) anyway.

	bhavana bajaj [:bajaj]
	Comment 19 • 11 years ago

Comment on attachment 702580 [details] [diff] [review]
patch

low risk fix needed for Bug 626775 when uplifted to aurora

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/58bbcf8b460a
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8d9ae8dbb72

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

Backed out for mochitest orange.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed6e1f9f2243

https://tbpl.mozilla.org/php/getParsedLog.php?id=19438037&tree=Mozilla-Aurora

12628 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug569988.html | uncaught exception - NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "winUtils.dispatchEventToChromeOnly is not a function" {file: "resource://gre/components/nsPrompter.js" line: 180}]' when calling method: [nsIPrompt::prompt] at http://mochi.test:8888/tests/editor/libeditor/text/tests/test_bug569988.html:39

	Frank Yan (:fryn) Assignee
	Comment 22 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/46d904f2f252
https://hg.mozilla.org/releases/mozilla-aurora/rev/c7115618ac6e