[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

#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?

Thanks for tracking down the regression range, Alice!

MozReview-Commit-ID: D6zaMJRxhXd

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?

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

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.

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...

[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

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')

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.

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

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.

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....

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

(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.

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

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?

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

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

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?

@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.

@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.

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.

> 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. :(

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+.

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.

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.

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!