Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro) Reporter
	Description • 7 years ago

[Affected versions]:
- 58.0a1 (2017-11-12)
- 57.0 build4 (20171112125346)
- 56.0.2 (20171024165158)

[Affected platforms]:
- Windows 10 x64 
- macOS 10.13

[Steps to reproduce]:
1. Launch Firefox
2. Go to https://ro.pinterest.com/, login using valid credentials and open a random item (photo/article)
3. Click the "View Site" (or "Read" or "Open") button from the bottom right side of the chosen item

[Expected result]:
- The original article/photo is properly opened in a new tab

[Actual result]:
- The original article/photo is opened in a new tab, but at a very small size
- The article/photo is then displayed a its right dimensions when redimensioning the browser window
- The issue is not fixed after a page refresh
- For more details, see the screencast https://goo.gl/cuyV9w

[Regression range]:
- It seems that the issue is not triggered on 55.0.3 build2 (20170824053622)
- I will provide more accurate info as soon as possible

[Additional notes]:
- Ubuntu platforms seems to be unaffected

	Alice0775 White
	Comment 1 • 7 years ago

#1Regression(Content in the new tab is empty until resize the browser):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4ef43b448dab296a3204077d6e9d0e8461759e77&tochange=85fb6e3ceb7a9521726d9316bf9f8d404b8de404


#2Partially improved(but tiny content until resize the browser):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b25d6223cf67202d09930defbf08e6bacb60fc4d&tochange=ea67db1628e0ad1f96bd67fc9f28197cecb71561


Regressed by: Bug 1343728
@:mystor, Your bunch of patch seems to cause the regression. Can you look into this?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 7 years ago

Thanks for tracking down the regression range, Alice!

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 3 • 7 years ago

MozReview-Commit-ID: D6zaMJRxhXd

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 7 years ago

Comment on attachment 8929523 [details] [diff] [review]
Process the CreateWindow reply message in order with other PContent IPC messages

> Bug 1416728 - Process the CreateWindow reply message in order with other PContent IPC messages, r=bz

Maybe explicitly say that MozPromise::Then ends up running callbacks off a separate runnable (unlike JS Promise) and hence can lead to the callbacks running after some other IPC messages are processed, even if those messages came after the one returning the MozPromise?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 5 • 7 years ago

Previously we used the MozPromise interface for calling an async-message over
IPC with a reply. Unfortunately, MozPromise processes the reply asynchronously,
potentially allowing other IPC messages to be processed before the `->Then`
callback is processed.

In the original CreateWindow patch I tried to work around this by setting the
target of the `->Then` callback to be StableStateEventTarget. This worked,
however as it isn't safe to run scripts etc. in the stable state, we instead
tried to exit the nested event loop immediately after the runnable ran, and then
performed processing of the reply.

Unfortunately, this bug exposed a problem with that design. If we have multiple
nested event loops then we cannot guarantee that we'll exit the nested event
loop immediately after recieving the `->Then` callback, which meant that we
could still process other IPC messages before we processed the CreateWindow
reply.

The fix to this was to add a new API to allow passing callbacks directly into
IPC send methods, ensure that those callbacks are called in IPC order, and
fully process the CreateWindow reply in the callback, rather than waiting for
the nested event loop to exit.

MozReview-Commit-ID: D6zaMJRxhXd

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 6 • 7 years ago

This problem is also in release right now. Do you think we'll want to uplift a fix to beta/release? The fix in the attached patch isn't a good one to uplift, as it depends on fairly large ipdl parser changes, but I could write a smaller fix which wouldn't be good to land in central, but could help fix this issue if we uplift it to beta.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 7 years ago

If we can have a safe enough fix, might be worth uplifting, yes.  We did ship this in 56, so it's not a new release regression...

	[:philipp]
	Comment 8 • 7 years ago

[Tracking Requested - why for this release]:
probably many people are seeing this on release for the first time now as they got e10s enabled, this is afairly noticeable bug to users & it got reported a number of times:
https://support.mozilla.org/en-US/questions/firefox?owner=all&tagged=bug1416728&show=all

	vueringschristian
	Comment 11 • 7 years ago

Is there anything we can do from the Pinterest side to help out with this?
Many of our users are reporting this bug and we're trying to figure out a good workaround.

Also note that it's reproducible with both
<a href="https://www.mozilla.org/en-US/" target="_blank">
and
window.open('https://www.mozilla.org/en-US/', '_blank')

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 7 years ago

There are some hints that possibly use of sync XHR is causing the issues here.
Sync XHR on the main thread is deprecated anyhow and will be removed from the browsers at some point because of the issues it causes to the user experience. But we'll keep debugging.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 13 • 7 years ago

This is a smaller and probably easier to uplift patch which _should_ also fix
the issue.

It works by taking advantage of how ipdl is implemented, and making a
non-compliant nsIEventTarget which just runs runnables as it is given them. This
allows us to run script when we recieve the message, allowing us to process the
response in order.

Unfortunately, I can't test to make sure that this patch works, because the
pinterest login dialog doesn't seem to work - I get an error every time I try to
log in in the console, and the UI doesn't update or navigate.

MozReview-Commit-ID: 2Ul99aDgPsD

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 14 • 7 years ago

I've confirmed that the problem locally is caused by a sync XHR which is being sent during the window.open call. This causes a nested event loop, which allows the events to be processed out of order. The stack looks something like this (mangled of course):

0 e(t = [object Object]) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":10]
1 r(e = [object Object]) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":10]
2 e(t = [object Object], n = [object Object], r = function(t){return e(t)}, o = function(e,r){return r?t(e):(o=e,n.again())}) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
    this = ContextLogResource(events=[object Object],[object Object], report_time=1511207051437000000)
3 n() ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
4 e/</c.start() ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
    this = function n(){return(l-=1)<0?t(o):(s.xhr=s._createXhr(a,i,function(t){return e(t)},function(e,r){return r?t(e):(o=e,n.again())}),s.xhr)}
5 e/<(e = function(n){return t.call(e,n)}, t = function(n){return t.call(e,n)}) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
6 t(e = function(e,t){var o=null,i={cache:!1,url:d,async:n,data:m,dataType:"json",headers:_,timeout:r,type:b?"GET":"POST"},a={url:d,requestData:m,resourceDict:p},c=function n(){return(l-=1)<0?t(o):(s.xhr=s._createXhr(a,i,function(t){return e(t)},function(e,r){return r?t(e):(o=e,n.again())}),s.xhr)};c.again=function(){C.a.setTimeout(c,u)},c.start=function(){c()},c.start()}) ["https://s.pinimg.com/webapp/js/vendor-react-e3f861e74bbd87065daa.js":4]
    this = [object Promise]
7 e(t = "create", n = false, r = null, o = [object Object], i = undefined) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
    this = ContextLogResource(events=[object Object],[object Object], report_time=1511207051437000000)
8 e(t = "create", n = [object Object], r = [object Object], o = undefined) ["https://s.pinimg.com/webapp/js/entryChunk-www-d2147c774cc7ed9b753b.js":22]
    this = ContextLogResource(events=[object Object],[object Object], report_time=1511207051437000000)
9 e(t = [object Object], n = [object Object])

If you can remove this sync XHR call, then the problem should go away.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 7 years ago

Comment on attachment 8930180 [details] [diff] [review]
BETA: Process the CreateWindow reply message in order with other PContent IPC messages

r=me, I think.  Hoping that running those bits while he IPDL message is being posted is in fact safe....

	Pulsebot
	Comment 16 • 7 years ago

Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea19dfc66ee0
Process the CreateWindow reply message in order with other PContent IPC messages, r=bz

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 17 • 7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> Comment on attachment 8930180 [details] [diff] [review]
> BETA: Process the CreateWindow reply message in order with other PContent
> IPC messages
> 
> r=me, I think.  Hoping that running those bits while he IPDL message is
> being posted is in fact safe....

I was just digging around inside of the IPDL code generator which generates the code we have - and this should have the same behaviour as the patch which I wrote for 59. It's definitely hacky and not good code to land if we don't have to.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 18 • 7 years ago

Comment on attachment 8930180 [details] [diff] [review]
BETA: Process the CreateWindow reply message in order with other PContent IPC messages

Approval Request Comment
[Feature/Bug causing the regression]: bug 1343728
[User impact if declined]: Some windows opened by pinterest will have the wrong dimensions until they are resized. There is a chance that pinterest will release a mitigation on their side which would avoid this issue without this patch being necessary. (This would involve them removing their sync XHR calls).
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: A similar fix has landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Please see comment 1 for STR.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: This change is somewhat risky. 
[Why is the change risky/not risky?]: It's different code than we're landing in nightly, as the changes we're landing in nightly are a bit large to uplift (we'd have to uplift all of bug 1418048), and it changes sensitive code around window opening.
[String changes made/needed]: None

	vueringschristian
	Comment 19 • 7 years ago

Nika - thank you for investigating and working on a fix!

At the moment we're still able to reproduce this bug in Nightly (59.0a1 (2017-11-20)) on pinterest.com.
We're using that XHR for context logging and won't be able to put a fix in soon (holiday freeze for thanksgiving).

Do you know how / when we'll be able to test out a Firefox build which includes the fix?

	Tiberius Oros[:tiberius_oros]
	Comment 20 • 7 years ago
bugherder

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

	Gerry Chang [:gchang]
	Comment 21 • 7 years ago

Hi :JuliaC,
Can you help check if this issue was fixed in the latest nightly?

	Julien Cristau [:jcristau]
	Comment 22 • 7 years ago

vueringschristian, current nightly should have the fix if you're able to test.

	vueringschristian
	Comment 23 • 7 years ago

gchang and jcristau I can confirm that this is now fixed in the latest nightly: 59.0a1 (2017-11-22). Thank you!
How long does it usually take for a patch like this to get into the stable release?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 24 • 7 years ago

@vueringschristian: That depends on whether or not this patch gets uplifted to beta/release. If the patch just rides the trains, Firefox 59 will be released 2018-03-13.

Another workaround which might be easier to execute on the pinterest side of things would be to set the `noopener` flag on your link. If you set that flag, we run the window opening logic through a different codepath, which won't trigger the dual-nested event loop problems which you're running into. It'll also be faster, so that's a bonus :-).

I noticed when looking at your code that you're already nulling out the opener in script immediately after opening the new tab, so I imagine you should be able to get away with it.

	vueringschristian
	Comment 25 • 7 years ago

@mystor: Thank you!

We just tried it out by using
`window.open(url, '_blank', 'noopener');`
instead of
`window.open(url, '_blank');`
which does work but it's opening those links in a new window instead of a new tab. Which is not the behavior our users expect.

Do you know of any other suggestions or workarounds to get this fixed?

Right now we're thinking of looking at whether the UserAgent matches 'Firefox/57' or 'Firefox/58' and if so, adding 'noopener' but this is less than ideal.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 26 • 7 years ago

Could you switch to using a normal anchor tag? like `<a href="url" target="_blank" rel="noopener">`? That should open the links in a new tab not a new window.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 7 years ago

> but it's opening those links in a new window instead of a new tab.

Yeah, that's https://github.com/whatwg/html/issues/1902

I filed bug 1419960 on getting that fixed.

It looks like nsWindowWatcher::CalculateChromeFlagsHelper never allows adding the CHROME_EXTRA flag, so there's actually no way to work around that bug with window.open either, that I can see, apart from using <a rel="noopener"> instead.  :(

	Gerry Chang [:gchang]
	Comment 28 • 7 years ago

Comment on attachment 8930180 [details] [diff] [review]
BETA: Process the CreateWindow reply message in order with other PContent IPC messages

Take this in Beta58 as this affects Pinterest users a lot. Beta58+.

	Narcis Beleuzu [:NarcisB]
	Comment 29 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/6b525d020e68

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/3efbbb4dc791

	Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro) Reporter
	Comment 32 • 7 years ago

I can confirm that the issue is verified fixed on 59.0a1 (2017-11-23)and 58.0b6 build1 (20171123161455), using Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.

	Sylvestre Ledru [:Sylvestre]
	Comment 33 • 7 years ago

Comment on attachment 8930180 [details] [diff] [review]
BETA: Process the CreateWindow reply message in order with other PContent IPC messages

As this isn't a new regression and Nika said that it is risky; I would rather let it ride the train with beta.

	vueringschristian
	Comment 35 • 7 years ago

We've also deployed a temporary fix for this on the Pinterest side so FireFox Quantum users on 57.0 will be able to get to external URLs again.

Thanks to everyone at FireFox for helping us debug this issue and coming up with a fix so quickly!