1126164 - Using window.open in allowed events handler will sometimes trigger popup blocker.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Hector Zhao [:hectorz]]

"sometimes" means:
1. On Fx 36+
2. window.open is called in the load handler of a sync ajax initiated in the event handler, as discussed in [1];
3. a plugin is loaded in the current browser window.

A minimal test case is available at [2], clicking "Sync!" on Fx 35 will open a new tab, but trigger a popup blocker on Fx 36+.

Regression range on Nightly channel is [3].

This technique is used by a major Chinese website alipay (similar to PayPal), so it's important to fix it before Fx 36 reach release.

[1]: http://stackoverflow.com/a/9514875
[2]: http://people.mozilla.org/~bzhao/sync-ajax-popup-blocker.html
[3]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=688f821edcd4&tochange=ab137ddd3746

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Hector, I assume <embed> in the testcase is needed?

I can't seem to reproduce this on Linux, but can on Windows.

My guess would be some of those plugin changes in the regression range.

Comment 2

[Hector Zhao [:hectorz]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Hector, I assume <embed> in the testcase is needed?

Yes, it's needed. However, it doesn't have to be in the same tab of the |window.open| call, a plugin in any tab within the same tabbrowser would be enough to trigger the popup blocker.

Comment 3

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=90731dbaab2d&tochange=6c63b3b1de80

Triggered by: Bug 669200

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking for site compat.

Olli - As you can reproduce the bug, can you take this bug?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar with Windows specific stuff and this is a regression from Bug 669200.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Olli.

Jim - You're the assignee on bug 669200. Can you take this one?

Comment 7

[Jim Mathies [:jimm]]

"Make the ajax call synchronous, which is something you should normally avoid like the plague as it locks up the UI of the browser."

not sure we would want this fixed after reading that, but ok! :)

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

This was introduced in this patch - 

https://bugzilla.mozilla.org/attachment.cgi?id=8521433&action=diff

Where I removed an optimization in win32 nsWindow's SetWindowClipRegion that early returned if the stored clip rects were equal to the incoming array. This triggered some additional WINDOWPOSCHANGING events, which triggered some funny code in the plugin instance subclass here - 

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowWin.cpp#352

which altered the global popup detection state.

Comment 9

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c901ee69bd3

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Comment 11

[Jim Mathies [:jimm]]

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

this regressed something with initial plugin window display + non-e10s.

Comment 12

[Jim Mathies [:jimm]]

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

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

::: widget/nsBaseWidget.cpp
@@ +646,5 @@
>  
>  bool
> +nsBaseWidget::IsWindowClipRegionEqual(const nsTArray<nsIntRect>& aRects)
> +{
> +  if (!aRects.Length() && !mClipRects) {

We can't do this, if mClipRects is null, GetWindowClipRegion returns the bounds, which triggers the regression I'm seeing. If mClipRects is null, we should return false.

Comment 13

[Jim Mathies [:jimm]]

Attachment: patch

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch r=roc

Comment 15

[Jim Mathies [:jimm]]

try push in comment 9.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c49c54cfb1d3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c49c54cfb1d3

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8556123 [details] [diff] [review]
patch r=roc

Approval Request Comment
[Feature/regressing bug #]:
regression caused by bug 669200
[User impact if declined]:
buggy popup blocking.
[Describe test coverage new/current, TreeHerder]:
on mc for a couple days.
[Risks and why]:
low risk, and we can't back out bug 669200, so we should fix. 
[String/UUID change made/needed]:
none.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Jim, any reason why you didn't ask for an uplift request to beta?

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8556123 [details] [diff] [review]
patch r=roc

Didn't realize this was needed on beta.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92211305e25b
https://hg.mozilla.org/releases/mozilla-beta/rev/e32f606d51e3

Comment 22

[Hector Zhao [:hectorz]]

Thanks!