Closed Bug 611553 Opened 9 years ago Closed 7 years ago

Make DOMWillOpenModalDialog a chrome-only event

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: Dolske, Assigned: fryn)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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. :)
Assignee: dolske → fryn
Status: NEW → ASSIGNED
Attachment #582168 - Flags: feedback?(ttaubert)
Attachment #582168 - Flags: feedback?(dolske)
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);
  },
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'.
Attachment #582168 - Flags: feedback?(ttaubert) → feedback+
(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. :)
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.
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).
(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?
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". :)
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.)
Attachment #582168 - Flags: feedback?(dolske)
Summary: Use notifications instead of DOM events for DOMWillOpenModalDialog → Make DOMWillOpenModalDialog a chrome-only event
Attachment #582168 - Attachment is obsolete: true
Depends on: 830858
Attached patch patchSplinter Review
Attachment #702580 - Flags: review?(dolske)
Attachment #702580 - Flags: review?(bugs)
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);
Attachment #702580 - Flags: review?(bugs) → review+
Comment on attachment 702580 [details] [diff] [review]
patch

Oorah!
Attachment #702580 - Flags: review?(dolske) → review+
Patching PromptService.js in /mobile/android/.
It depends on the other patch.
Attachment #702683 - Flags: review?(mbrubeck)
Blocks: 831166
Oops, uploaded the wrong file. Sorry.
Attachment #702683 - Attachment is obsolete: true
Attachment #702683 - Flags: review?(mbrubeck)
Attachment #702690 - Flags: review?(mbrubeck)
Attachment #702690 - Flags: review?(mbrubeck) → review+
Some test would be really nice.
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!
Target Milestone: --- → mozilla21
Version: unspecified → Trunk
Attachment #702580 - Flags: approval-mozilla-aurora?
Attachment #702690 - Flags: approval-mozilla-aurora?
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.
Comment on attachment 702580 [details] [diff] [review]
patch

low risk fix needed for Bug 626775 when uplifted to aurora
Attachment #702580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #702690 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
You need to log in before you can comment on or make changes to this bug.