All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: al_9x, Assigned: mrbkap)

Tracking

Trunk
Firefox 41
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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
perhaps regressed by Bug 570266?
Flags: needinfo?(smacleod)
Much more likely to be bug 846635.
Blocks: 846635
Flags: needinfo?(smacleod)
(Reporter)

Updated

4 years ago
Flags: needinfo?(raymond)
Mano, any thoughts on ways to avoid this?
Flags: needinfo?(raymond) → needinfo?(mano)
(Reporter)

Comment 5

4 years ago
Is there anyone besides mano@mozilla.com who can identify the problem commit?
The problem commit is known, what's needed is a suggested fix.
(Reporter)

Comment 7

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

Comment 9

4 years ago
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
(Reporter)

Comment 10

4 years ago
Created attachment 8474161 [details] [diff] [review]
patch.patch

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

Comment 11

4 years ago
Those who introduced this, are not responding, why?

I proposed a solution and a patch, presumably someone needs to review it, who?
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?
Just tested myself, it doesn't seem to. I wonder why?
(Reporter)

Comment 14

4 years ago
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
(Reporter)

Comment 15

4 years ago
just realized that you probably meant directly, I'll look into it
(Reporter)

Comment 16

4 years ago
Created attachment 8475607 [details] [diff] [review]
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.
Attachment #8474161 - Attachment is obsolete: true
(Reporter)

Comment 17

4 years ago
is the latest patch acceptable?  can this move forward?
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).
(Reporter)

Comment 19

4 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mano) → needinfo?(gavin.sharp)
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.
Attachment #8475607 - Flags: feedback?(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?
Attachment #8475607 - Flags: feedback?(bzbarsky) → feedback+
Yes, but I don't assume there's any way Task.jsm could accomplish that on its own, right?
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.
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.
That is, unless you think this super-scary methods would be useful for other JS code.
I don't think it would, offhand.
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.
(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.
Flags: needinfo?(gavin.sharp)
(Reporter)

Comment 30

4 years ago
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.
(Reporter)

Comment 31

4 years ago
Created attachment 8500131 [details] [diff] [review]
popups-fx31.patch

for Fx31
Created attachment 8507579 [details] [diff] [review]
patch

Here's an updated patch. We really should have a test for this before committing it, though.
Attachment #8475607 - Attachment is obsolete: true
Attachment #8500131 - Attachment is obsolete: true
(Reporter)

Comment 33

4 years ago
Why isn't this getting merged?
(Reporter)

Comment 35

4 years ago
Are you going to add the test?
(Reporter)

Comment 36

4 years ago
Why is it better for this to remain broken rather be fixed without a test?
(Reporter)

Comment 37

4 years ago
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.
I have to admit to a certain empathy for the question raised in comment 36.... ;)
Flags: needinfo?(gavin.sharp)
(Reporter)

Comment 39

3 years ago
Gavin Sharp is leaving Mozilla, can some one else merge this?
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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 41

3 years ago
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

3 years ago
Created attachment 8614122 [details] [diff] [review]
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.

Updated

3 years ago
Attachment #8507579 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)

Updated

3 years ago
Duplicate of this bug: 1079122

Comment 46

3 years ago
Blake, do you think you have time to figure out why the popup state remains how it is?
Flags: needinfo?(mrbkap)

Comment 47

3 years ago
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
Created attachment 8615577 [details] [diff] [review]
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 on attachment 8615577 [details] [diff] [review]
Make the new test pass.

This patch applies on top of attachment 8614122 [details] [diff] [review].
Flags: needinfo?(mrbkap)
Attachment #8615577 - Flags: review?(gijskruitbosch+bugs)
(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.
Assignee: nobody → mrbkap

Comment 51

3 years ago
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);
});
Attachment #8615577 - Flags: review?(gijskruitbosch+bugs) → review+
Depends on: 1172204
Created attachment 8616329 [details] [diff] [review]
What I'm going to check in

This includes all of the fixes and addresses the comment.
Attachment #8614122 - Attachment is obsolete: true
Attachment #8615577 - Attachment is obsolete: true
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
note startsWith is not case insensitive, but all those urls come from nsIURI or URL, so the scheme should always be lowercase
Created attachment 8621648 [details] [diff] [review]
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
Attachment #8616329 - Attachment is obsolete: true
Keywords: checkin-needed
"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
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.
it's also strange that in the next test we get a stack from openKeywordBookmarkWithWindowOpen

Updated

3 years ago
See Also: → bug 1174466
Created attachment 8622095 [details] [diff] [review]
Really fix tests
Attachment #8621648 - Attachment is obsolete: true
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.
Attachment #8622095 - Flags: review?(mak77)
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.
Attachment #8622095 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/f50a771d7d1b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

3 years ago
Duplicate of this bug: 1180342
You need to log in before you can comment on or make changes to this bug.