1044351 - pop-up blocker is blocking tabs opened by parameterized keyword bookmarklets (Fx 25-34)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[al_9x]

works in Fx 24
the bookmarklet must have a parameter (%s) (works without one)
must be invoked by keyword (works if clicked)

1. new profile
2. create a bookmarklet
"javascript:void open('https://www.google.com/search?q=%s');"
3. invoke by keyword (argument not required to repro)
4. firefox prevented this site from opening a pop-up window

Comment 1

[al_9x]

Last good revision: d7553251cf43 (2013-07-01)
First bad revision: 23ce4eab8fb1 (2013-07-02)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d7553251cf43&tochange=23ce4eab8fb1

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

perhaps regressed by Bug 570266?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Much more likely to be bug 846635.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Mano, any thoughts on ways to avoid this?

Comment 5

[al_9x]

Is there anyone besides [e-mail] who can identify the problem commit?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

The problem commit is known, what's needed is a suggested fix.

Comment 7

[al_9x]

So it's definitely bug 846635 (http://hg.mozilla.org/mozilla-central/rev/19845a51b863)?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Yes.

Comment 9

[al_9x]

this process starts with

#urlbar.handleCommand

which from an async callback calls

openUILinkIn

which calls

openLinkIn

which calls

loadURIWithFlags

which can take LOAD_FLAGS_ALLOW_POPUPS

so the fix is:

openUILinkIn takes a params object
an additional param should be introduced (allowPopups) which handleCommand will pass to it
openUILinkIn forwards the params to openLinkIn 
which translates them to LOAD_FLAGS_*
openLinkIn will need to add LOAD_FLAGS_ALLOW_POPUPS based on allowPopups

Comment 10

[al_9x]

Attachment: patch.patch

This patch is from a modified omni.ja of the latest nightly

Comment 11

[al_9x]

Those who introduced this, are not responding, why?

I proposed a solution and a patch, presumably someone needs to review it, who?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8474161 [details] [diff] [review]
patch.patch

With this patch, presumably using a bookmark keyword to open http://www.popuptest.com/popuptest1.html allows popup windows to be opened, which isn't desirable and I believe would be a regression compared to Firefox 24 behavior.

Can you confirm?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Just tested myself, it doesn't seem to. I wonder why?

Comment 14

[al_9x]

the url that is allowed to open popups via LOAD_FLAGS_ALLOW_POPUPS is the javascript: bookmarklet url, the popups that are opened by http://www.popuptest.com/popuptest1.html are the grandchildren of the LOAD_FLAGS_ALLOW_POPUPS load, they don't inherit it.

javascript:open('http://www.popuptest.com/popuptest1.html') - passed LOAD_FLAGS_ALLOW_POPUPS 
http://www.popuptest.com/popuptest1.html tab - is the a popup allowed by LOAD_FLAGS_ALLOW_POPUPS 
any popups opened by it should not be allowed

Comment 15

[al_9x]

just realized that you probably meant directly, I'll look into it

Comment 16

[al_9x]

Attachment: popups.patch

not clear why it blocks, didn't see anything obvious on the js side, but it doesn't make sense to depend on this mysterious behavior, this is only intended for javascript: bookmarklets.

Comment 17

[al_9x]

is the latest patch acceptable?  can this move forward?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Ideally the next steps are to understand why exactly this regressed, and why that patch fixes it (and why it doesn't have the effect I expected in comment 12).

Comment 19

[al_9x]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #18)
> Ideally the next steps are to understand why exactly this regressed,

it regressed because the navigation became asynchronous to the the event that triggered it

now perhaps the popup blocker should be smart enough to still allow it, and maybe it's a bug that it does not, but let's not wait indefinitely for that to be figured out and addressed.

> and why
> that patch fixes it (and why it doesn't have the effect I expected in
> comment 12).

That LOAD_FLAGS_ALLOW_POPUPS does not allow popups for an http navigation, would seem like a bug, perhaps a new one should be filed for that.  Bur why should this bug block on that?

LOAD_FLAGS_ALLOW_POPUPS does allow popups for javascript: urls, which per its documentation, is normal, expected behavior, and the only thing I rely on in the new version of the patch.

LOAD_FLAGS_ALLOW_POPUPS 	32768 	This flag specifies that the load should not be subject to popup blocking checks.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebNavigation#Load_Flags

Comment 20

[Asaf Romano (gone)]

I tend to agree. The fix seems OK to me at first glance.

Gavin, why do you expect LOAD_FLAGS_ALLOW_POPUPS to be forwarded to the next load (the javascript "navigation" is different from the window.open one)?

The flag doesn't stick: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4633

By the way, Boris Zbarsky suggested doing something along these lines in Bug 275071 comment 4.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 8475607 [details] [diff] [review]
popups.patch

Boris, does this change seem reasonable? This sets the LOAD_FLAGS_ALLOW_POPUPS flag for every by-the-ui "javascript:" load. It's necessary because those loads are no longer done synchronously from the onclick/onkeypress/oncommand listener.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8475607 [details] [diff] [review]
popups.patch

This seems fine to me if that's the UI behavior we want.

It might make sense to fix the underlying async code here, though.  It's using Task.spawn, right?  Should that be carrying through the popup control state, the way setTimeout of less than "dom.disable_open_click_delay" ms does?

Comment 23

[Asaf Romano (gone)]

Yes, but I don't assume there's any way Task.jsm could accomplish that on its own, right?

Comment 24

[Boris Zbarsky [:bzbarsky]]

It could if it used a delay primitive (implemented in C++, presumably, though we could add some super-scary methods to make this possible in JS) that saved/restored the popup control state.

Comment 25

[Asaf Romano (gone)]

I see. This sounds like an overkill for the arguably edge case we're dealing with here, and not much less hacky than "manually" passing over the allow-popups flag between the Task.spawn caller to the generator code itself.

Comment 26

[Asaf Romano (gone)]

That is, unless you think this super-scary methods would be useful for other JS code.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I don't think it would, offhand.

Comment 28

[Asaf Romano (gone)]

OK, thanks. I'll file a bug nonetheless because I don't think I'm the one to make the decision, but for now I think it's a good idea to take this patch.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #20)
> Gavin, why do you expect LOAD_FLAGS_ALLOW_POPUPS to be forwarded to the next
> load (the javascript "navigation" is different from the window.open one)?

I don't, the concern was about it affecting all first loads. The latest patch (limiting to javascript: loads) mitigates that mostly.

I'd feel better fixing this properly, but I won't object to landing this.

Comment 30

[al_9x]

I am not set up for fx dev, can someone please apply this.

Considering this regressed in an ESR 31, it would be good to fix it there too.

Comment 31

[al_9x]

Attachment: popups-fx31.patch

for Fx31

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Here's an updated patch. We really should have a test for this before committing it, though.

Comment 33

[al_9x]

Why isn't this getting merged?

Comment 34

[:Gavin Sharp [email: [e-mail]]]

See comment 32.

Comment 35

[al_9x]

Are you going to add the test?

Comment 36

[al_9x]

Why is it better for this to remain broken rather be fixed without a test?

Comment 37

[al_9x]

This functionality worked probably from the inception of Fx, for more than a decade.  The bug, analysis and a solution were brought to you on a silver platter and still you won't lift a finger.  What, Chrome doesn't have this, so why bother?  Single digits are just around the corner, just keep breaking things, removing useful functionality and otherwise copying Chrome, you'll get there in no time.

Comment 38

[Boris Zbarsky [:bzbarsky]]

I have to admit to a certain empathy for the question raised in comment 36.... ;)

Comment 39

[al_9x]

Gavin Sharp is leaving Mozilla, can some one else merge this?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Let's try a random Firefox peer...

Please see comment 36 for the context here: we have a patch, but it's not being landed just because we haven't written an automated test for it yet.  That's OK as long as there's a credible plan for writing the test soon, but that doesn't seem to be the case here.

Comment 41

[:Gijs (he/him)]

Sorry for the delay here.

I can look at doing up a test, but it'll have to be tomorrow.

Considering the number of bugs with this kind of stuff (namely, window opening), I would really prefer this to land with test and not without.

Comment 42

[:Gijs (he/him)]

Attachment: prevent popup blocker from blocking windows opened by loaded javascript: URIs by allowing popups for such loads from the location bar

So unfortunately, I wrote a test and it breaks. The 'allow popups' flag does affect the window that gets opened, and so in this test, two tabs open. I don't think this patch should land like this, and I don't have the time to investigate this further. The next step would be figuring out why this is leaving the popup state as 'allowed' for the window open.

Comment 43

[:Gijs (he/him)]

Gah. So I just spent at least 30 minutes thinking this didn't work because of how the JS protocol doesn't work on the new tab page...

Comment 44

[:Gijs (he/him)]

(last comment is true but outdated; wanted to clear needinfo flags, sorry for the spam)

Comment 46

[:Gijs (he/him)]

Blake, do you think you have time to figure out why the popup state remains how it is?

Comment 47

[fa]

I slightly disagree that #1079122 is duplicate.

this one:
> the bookmarklet must have a parameter (%s) (works without one)
> must be invoked by keyword (works if clicked)

the other one:
> has no parameter
> doesn't work when clicked.

So yes, it may be the same reason, but the reproduce steps and preconditions are somewhat different

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make the new test pass.

Luckily, there was no crazy propagation to debug here. Instead, we actually disable the popup blocker for mochitests. In order to ensure that it still works, we have to explicitly re-enable it.

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8615577 [details] [diff] [review]
Make the new test pass.

This patch applies on top of attachment 8614122 [details] [diff] [review].

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to fa from comment #47)
> So yes, it may be the same reason, but the reproduce steps and preconditions
> are somewhat different

The preconditions are, in fact, very different. Bug 1079122 involves e10s, which makes bookmark interactions even more asynchronous. The fix here should fix both cases.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8615577 [details] [diff] [review]
Make the new test pass.

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

Gah. Why is that pref even set to false for mochitests? :-\

Also, in that case, have you verified that this test still fails without the fix applied?

::: browser/components/places/tests/browser/browser_bookmarklet_windowOpen.js
@@ +20,5 @@
> +                                      [ 'dom.disable_open_during_load', true ]] },
> +                            function() {
> +                              deferred.resolve();
> +                            });
> +  yield deferred.promise;

nit:

yield new Promise((resolve, reject) => {
  SpecialPowers.pushPrefEnv(..., resolve);
});

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm going to check in

This includes all of the fixes and addresses the comment.

Comment 53

[Marco Bonardo [:mak]]

Comment on attachment 8616329 [details] [diff] [review]
What I'm going to check in

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

::: browser/base/content/urlbarBindings.xml
@@ +351,5 @@
>                  allowThirdPartyFixup: true,
>                  disallowInheritPrincipal: !mayInheritPrincipal,
>                  allowPinnedTabHostChange: true,
> +                postData: postData,
> +                allowPopups: /^javascript:/i.test(url),

nit: url.startsWith("javascript:")

::: browser/components/places/PlacesUIUtils.jsm
@@ +867,5 @@
>          }
>        }
>  
>        aWindow.openUILinkIn(aNode.uri, aWhere, {
> +        allowPopups: /^javascript:/i.test(aNode.uri),

ditto

Comment 54

[Marco Bonardo [:mak]]

note startsWith is not case insensitive, but all those urls come from nsIURI or URL, so the scheme should always be lowercase

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/298c695564e9

Comment 56

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c1afb1408755 for a string of browser-chrome timeouts, https://treeherder.mozilla.org/logviewer.html#?job_id=10694252&repo=mozilla-inbound

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For checkin

There was a typo in the previous patch. This one is green on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cda8b92d9f9

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a1a682a3d489

Comment 59

[Phil Ringnalda (:philor)]

"Fun" one for this backout. Nearly all the time on Win7 PGO in bc1 and e10s bc3, most of the time in Win8 PGO bc1, and about half the time in Win8 opt non-PGO bc1 (so, "more often the faster we run on Windows"), https://treeherder.mozilla.org/logviewer.html#?job_id=3446446&repo=fx-team

20:21:40 INFO - 385 INFO End of test: Bug 491269 - Test that editing folder name in bookmarks properties dialog does not accept the dialog
20:21:40 INFO - 386 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmarksProperties.js | A promise chain failed to handle a rejection: - at resource://gre/modules/Sqlite.jsm:800 - Error: Error(s) encountered during statement execution: cannot commit - no transaction is active
20:21:40 INFO - Stack trace:
20:21:40 INFO - ConnectionData.prototype<._executeStatement/pending<.handleCompletion@resource://gre/modules/Sqlite.jsm:800:25
20:21:40 INFO - ConnectionData.prototype<.executeTransaction/promise<@resource://gre/modules/Sqlite.jsm:551:32
20:21:40 INFO - Promise*ConnectionData.prototype<.executeTransaction@resource://gre/modules/Sqlite.jsm:642:30
20:21:40 INFO - OpenedConnection.prototype<.executeTransaction@resource://gre/modules/Sqlite.jsm:1348:12
20:21:40 INFO - insertBookmark/<@resource://gre/modules/Bookmarks.jsm:777:11
20:21:40 INFO - ConnectionData.prototype<.executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:362:25
20:21:40 INFO - OpenedConnection.prototype<.executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:1214:12
20:21:40 INFO - this.PlacesUtils.withConnectionWrapper/<@resource://gre/modules/PlacesUtils.jsm:1420:14
20:21:40 INFO - ConnectionData.prototype<.executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:372:1
20:21:40 INFO - OpenedConnection.prototype<.executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:1214:12
20:21:40 INFO - this.PlacesUtils.withConnectionWrapper/<@resource://gre/modules/PlacesUtils.jsm:1420:14
20:21:40 INFO - remove@resource://gre/modules/Bookmarks.jsm:393:1
20:21:40 INFO - openKeywordBookmarkWithWindowOpen/<@chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmarklet_windowOpen.js:38:5
20:21:40 INFO - Tester.prototype.nextTest<@chrome://mochikit/content/browser-test.js:423:17
20:21:40 INFO - testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1049:11
20:21:40 INFO - testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:951:9

Backed out in https://hg.mozilla.org/integration/fx-team/rev/17f159b29970

Comment 60

[Marco Bonardo [:mak]]

maybe you should wait for the bookmark and keyword removal in the cleanup function (should support a returned promise so you should be able to pass a Task.async)


the error looks fancy though and seems to point at a coding mistake in Sqlite.jsm... indeed we should surely not try to commit a non existing transaction.

Comment 61

[Marco Bonardo [:mak]]

it's also strange that in the next test we get a stack from openKeywordBookmarkWithWindowOpen

Comment 62

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Really fix tests

Comment 63

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8622095 [details] [diff] [review]
Really fix tests

I don't think we need to use Task.async here, as cleanup functions are already in a Task.async and we yield the result of calling the function.

Comment 64

[Marco Bonardo [:mak]]

Comment on attachment 8622095 [details] [diff] [review]
Really fix tests

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

Regardless, please file a bug in toolkit / places with the stack in comment 59, it looks very suspect and should be investigated sooner or later.

Comment 65

[Pulsebot]

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

Comment 66

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 1175578.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

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