1079122 - [e10s] Bookmarklet is wrongfully labelled as pop-up window originating from current site

Status: RESOLVED DUPLICATE of bug 1044351

(Firefox :: Bookmarks & History, defect)

Description

[fa]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Make a bookmarklet like the one from https://pinboard.in

javascript:q=location.href;if(document.getSelection){d=document.getSelection();}else{d='';};p=document.title;void(open('https://pinboard.in/add?url='+encodeURIComponent(q)+'&description='+encodeURIComponent(d)+'&title='+encodeURIComponent(p),'Pinboard','toolbar=no,width=700,height=350'));

This is 35.0a1 (2014-10-06) on Debian Wheezy.


Actual results:

Nightly prevented this site from opening a pop-up window.


Expected results:

A popup should appear, like on Nightly from a few weeks ago.
Sadly I can't tell which version of Nightly that was. Unless there is some update log in the firefox folder, then I might be able to look it up.

Comment 1

[:Gijs (he/him)]

(In reply to fa from comment #0)
> A popup should appear, like on Nightly from a few weeks ago.
> Sadly I can't tell which version of Nightly that was. Unless there is some
> update log in the firefox folder, then I might be able to look it up.

There is probably an update log somewhere (not 100% sure where on Debian, esp. if this is a distro rather than official build) but perhaps you could try using mozregression to track this down? More info here:

http://mozilla.github.io/mozregression/ 

If it was really only a few weeks that this was broken, I imagine something like:

mozregression --good 2014-08-18

should find out when this broke in very little time, and it'd be a big help in fixing this. :-)

(if this finds that the 2014-08-19 nightly was also bad, perhaps you need to set --good to an earlier date and it wasn't quite a "few" weeks... :-) )

Comment 2

[fa]

This is not a Debian build, it's the official nighlty I downloaded from Mozilla.
I have a file updated/updates.xml in here but I can't say which version is the correct one that was running before (had an uptime of 19 days on this laptop, and I update nightly only every few weeks.

Does this help? I don't see if there is a flag for "this was installed and not just noticed"

http://sprunge.us/fOfi

Comment 3

[Paul Silaghi, QA [:pauly]]

Find a nightly build that worked fine here ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/.
Then install mozregression and run mozregression --good 2014-xx-xx.
Post back here the resulting pushlog.

Comment 4

[Petruta Horea [:phorea], Desktop QA]

Is e10s enabled when this happens?

Comment 5

[fa]

yes, e10s is enabled (I am on Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0) meanwhile

Deactivating e10s fixes this.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch

The problem here is that we handle the click in the parent process and then send an asynchronous message to the child simply loading the URL.  By the time we create the JS channel for the load, we've lost the fact that we're handling a click and block the popup.

I must admit that I'm pretty lost in this code. This patch seems like the easiest way to fix the bug: I'm assuming that the user must interact (either with the mouse or keyboard) with the Places node in order to trigger _openNodeIn, so I simply pass along the "allow popups" flag.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8610840 [details] [diff] [review]
Possible patch

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

It looks harmless and simple enough.

::: browser/components/places/PlacesUIUtils.jsm
@@ +868,5 @@
>        }
>  
>        aWindow.openUILinkIn(aNode.uri, aWhere, {
>          inBackground: Services.prefs.getBoolPref("browser.tabs.loadBookmarksInBackground"),
> +        allowPopups: true,

could you please add a brief comment explaining we allow popups to properly support bookmarklets?

Comment 8

[:Gijs (he/him)]

Err... well, that's funny. This is a dupe, and has an identical patch, but no test, and the test I wrote when asked to showed that this causes popups to remain allowed for the window that's opened. I think the patch needs more work. I'll dupe in a second.

Comment 9

[:Gijs (he/him)]

Blake, maybe you want to take the patch further in bug 1044351, including the test and/or fixing why this is propagating "popups allowed" state? :-)