Closed Bug 1095236 Opened 10 years ago Closed 9 years ago

[e10s] window.open(..., ..., "dialog=1") breaks with e10s enabled

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
e10s m8+ ---
firefox44 --- fixed

People

(Reporter: chmanchester, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files)

No longer blocks: 1092223
Bug 1095260 removed the dialog=1 property from the test-file, so your link doesn't reproduce the bug anymore.
http://hg.mozilla.org/mozilla-central/filelog/502e1a5e722f/testing/marionette/client/marionette/www/test_windows.html


Here's a jsfiddle which should show the bug: http://jsfiddle.net/as03ohoc/39/

> 10:11:23.226 NS_ERROR_FAILURE:  show:27:0

Browser Console and debug-console come up empty.
The issue here is we don't call ProvideWindow because the dialog flag is set at http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#647

This causes us to try to open a window from the child process.

It does work if I remove that check.
According to https://developer.mozilla.org/en-US/docs/Web/API/Window/open, "dialog=1" is only supported by Mozilla browsers.

I wonder if it's just some copy-paste that's been propagated around since around forever.

We might want to revisit where we've milestoned this. I don't think we have a good idea of how many sites might be attempting to open popups with dialog=1.
This appears to be working now. Please re-open if this is still an issue.
Assignee: nobody → mconley
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Aaaaand, I just reproduced this with the jsfiddle in comment 1.

Re-nomming.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee: mconley → mrbkap
I've had some window-opening stuff on my plate lately, so it's kinda swapped into my head already. I'm going to tentatively steal this, but do let me know if you'd like it back.
Assignee: mrbkap → mconley
Status: REOPENED → ASSIGNED
Bug 1095236 - Simplify browser_test_new_window_from_content to use BrowserTestUtils.
Bug 1095236 - Disable dialog=1 support for windows opened from content.
Bug 1095236 - Test that windows opened from content with dialog=1 still open.
So the patch I've got here causes us to ignore dialog=1 when window.open is called from content.

My justification for this is that dialog=1 really only affects the appearance of the popups (by hiding the command menu and the min/max/restore buttons), but otherwise does nothing else to change the behaviour of the popup. Also, Gecko is the only engine to support this, and I doubt anyone else ever will.

I can't think of any good reason, honestly, to keep dialog=1. I suspect that if we found code out there that was using dialog=1, it was probably copy-paste and is not something the site relies on... because I honestly cannot understand how dialog=1 could be useful to the web.
Comment on attachment 8669232 [details]
MozReview Request: Bug 1095236 - Simplify browser_test_new_window_from_content to use BrowserTestUtils.

Bug 1095236 - Simplify browser_test_new_window_from_content to use BrowserTestUtils.
Attachment #8669232 - Flags: review?(mrbkap)
Comment on attachment 8669233 [details]
MozReview Request: Bug 1095236 - Disable dialog=1 support for windows opened from content.

Bug 1095236 - Disable dialog=1 support for windows opened from content.
Attachment #8669233 - Flags: review?(mrbkap)
Attachment #8669234 - Flags: review?(mrbkap)
Comment on attachment 8669234 [details]
MozReview Request: Bug 1095236 - Test that windows opened from content with dialog=1 still open.

Bug 1095236 - Test that windows opened from content with dialog=1 still open.
Comment on attachment 8669233 [details]
MozReview Request: Bug 1095236 - Disable dialog=1 support for windows opened from content.

https://reviewboard.mozilla.org/r/21155/#review19341

Looks good. That test is so much more readable now!
Attachment #8669233 - Flags: review?(mrbkap) → review+
Comment on attachment 8669232 [details]
MozReview Request: Bug 1095236 - Simplify browser_test_new_window_from_content to use BrowserTestUtils.

https://reviewboard.mozilla.org/r/21153/#review19343
Attachment #8669232 - Flags: review?(mrbkap) → review+
Comment on attachment 8669234 [details]
MozReview Request: Bug 1095236 - Test that windows opened from content with dialog=1 still open.

https://reviewboard.mozilla.org/r/21157/#review19345
Attachment #8669234 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a179310161fb9240245995f86a31ef45cace38f6
Bug 1095236 - Simplify browser_test_new_window_from_content.js to use BrowserTestUtils. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/856b7b90184f29a64093970e540193731b963f61
Bug 1095236 - Disable dialog=1 support for windows opened from content. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/11cb6379251ae9efd70bf3bc1f8fab9b66b3d964
Bug 1095236 - Test that windows opened from content with dialog=1 still open. r=mrbkap.
I backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d2282c3bfb43 on the off chance this caused a spike in VP(b-m) failures on inbound
If that spike doesn't go away with this backout, feel free to reland whenever.
Flags: needinfo?(mconley)
The spike happened elsewhere also, so this can't be at fault. 

And even if it was at fault, we don't back out patches for tier-2 test failures:

Relanded: 
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e074f9eab4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6be3322df9e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c29770290ac1
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/c5e074f9eab4
https://hg.mozilla.org/mozilla-central/rev/6be3322df9e4
https://hg.mozilla.org/mozilla-central/rev/c29770290ac1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Now also added to Firefox 44 for developers.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: