Closed Bug 1044351 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: al_9x, Assigned: mrbkap)

References

Details

Attachments

(1 file, 8 obsolete files)

Really fix tests
8.87 KB, patch
mak
: review+
Details | Diff | Splinter Review 
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
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)
Flags: needinfo?(raymond)
Mano, any thoughts on ways to avoid this?
Flags: needinfo?(raymond) → needinfo?(mano)
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.
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
Attached patch patch.patch (obsolete) — — Splinter Review 
This patch is from a modified omni.ja of the latest nightly
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?
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
just realized that you probably meant directly, I'll look into it
Attached patch popups.patch (obsolete) — — Splinter Review 
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
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).
(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)
Attachment #8475607 - Flags: review+
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.
Attached patch popups-fx31.patch (obsolete) — — Splinter Review 
for Fx31
Attached patch patch (obsolete) — — Splinter Review 
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
Why isn't this getting merged?
Are you going to add the test?
Why is it better for this to remain broken rather be fixed without a test?
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)
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)
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.
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.
Attachment #8507579 - Attachment is obsolete: true
Blake, do you think you have time to figure out why the popup state remains how it is?
Flags: needinfo?(mrbkap)
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
Attached patch Make the new test pass. (obsolete) — — Splinter Review 
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 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
Attached patch What I'm going to check in (obsolete) — — Splinter Review 
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
Attached patch For checkin (obsolete) — — Splinter Review 
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
See Also: → 1174466
Attached patch Really fix tests — — Splinter Review 

        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
Closed: 9 years ago

          status-firefox41:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: