Fix issue #49

[joeduncan]

Fixes issue #49 Unless there is a better way to fix it ;)

[defunctzombie]

@calvinmetcalf please take a look

[calvinmetcalf]

looking at it now

[calvinmetcalf]

see my comment on #49, basically if currentQueue is null then an assumption we are making is being violated, from reading #49 it sounds like setTimeout(fn, 0) === process.nextTick(fn) which would be madness, but if that is indeed the case there may be an easier solution

[defunctzombie]

So this is not an appropriate fix? Is it maybe good enough or is there a deeper issue?

On Tuesday, September 8, 2015, Calvin Metcalf notifications@github.com wrote:

see my comment on #49 https://github.com/defunctzombie/node-process/issues/49, basically if currentQueue is null then an assumption we are making is being violated, from reading #49 https://github.com/defunctzombie/node-process/issues/49 it sounds like setTimeout(fn, 0) === process.nextTick(fn) which would be madness, but if that is indeed the case there may be an easier solution

— Reply to this email directly or view it on GitHub https://github.com/defunctzombie/node-process/pull/50#issuecomment-138607327 .

[calvinmetcalf]

adding a 4 to this line might fix it too, but this will probably as well, I'd like to understand how we can get into this situation in the first place because it sounds like setTimeout is calling process.nextTick here somehow

[joeduncan]

I managed to reproduce it by opening a new window with window.open. draining suddenly becomes false in parent window while executing both while loops. I have a strange feeling that Firefox may execute same code via window.opener (in new window) as this would reference the parent window.

Just tried setting var timeout = setTimeout(cleanUpNextTick, 4);. Doesn't seem to solve the problem.

[calvinmetcalf]

@joeduncan fascinating are you able to write a gist?

[joeduncan]

@calvinmetcalf i just did some more debugging, it does not seem to have anything to do with opening a new window (except for maybe slowing it down). It seems that Chrome does not executecleanUpNextTick here var timeout = setTimeout(cleanUpNextTick, 0);, whereas Firefox does, and thus calls drainQueue() again and tries to drain the same queue. The question is here is why exactly is currentQueue added back to queue here ?https://github.com/defunctzombie/node-process/blob/master/browser.js#L12 This means that it there is anything left in currentQueue those items will be run twice? Leaving away the whole if/else from L12 and keeping currentQueue inside the drainQueue function scope should work fine, or am i missing something here?

[calvinmetcalf]

So the idea is that if there is an error thrown by one of the functions then that is the only way that the clear timeout function after the loop won't run, so we combine the current queue which are functions enqueued last run before the error with the current queue which will be the functions we havn't gotten to and re run it.

I don't understand how drainQueue can pause and the timeout be called in the middle of the loop unless some how set timeout is calling this exact same process nextTick function, but that makes no sense either unless Babel or Webpack or react or an add on is overwriting set timeout somehow

On Tue, Sep 8, 2015, 4:30 PM Joe Duncan notifications@github.com wrote:

@calvinmetcalf https://github.com/calvinmetcalf i just did some more debugging, it does not seem to have anything to do with opening a new window (except for maybe slowing it down). It seems that Chrome does not executecleanUpNextTick here var timeout = setTimeout(cleanUpNextTick, 0);, whereas Firefox does, and thus calls drainQueue() again and tries to drain the same queue. The question is here is why exactly is currentQueue added back to queue here ? https://github.com/defunctzombie/node-process/blob/master/browser.js#L12 This means that it there is anything left in currentQueue those items will be run twice? Leaving away the whole if/else from L12 and keeping currentQueue inside the drainQueue function scope should work fine, or am i missing something here?

— Reply to this email directly or view it on GitHub https://github.com/defunctzombie/node-process/pull/50#issuecomment-138690627 .

[joeduncan]

I created a short example (with fluxible) to reproduce error in Firefox, https://github.com/joeduncan/node-process-example

npm install
npm run dev

http://localhost:3000/ -> click on open new window

[calvinmetcalf]

excellent thank you testing

[defunctzombie]

So what is the final verdict here folks? Is this patchup the solution or will someone be researching a core issue fix here to avoid racy problems from coming up?

[calvinmetcalf]

I'm trying to reproduce now

[calvinmetcalf]

the following triggers it

<html>
<head>
    <meta charSet="utf-8" />
    <title>title</title>
    <meta name="viewport" content="width=device-width, user-scalable=no" />
    <link rel="stylesheet" href="http://yui.yahooapis.com/pure/0.5.0/pure-min.css" />
</head>
<body>

</body>
<script>
var called = false;
setTimeout(function (){
  console.log('should be true', called);
});
console.log('opening');
window.open('http://example.com/some/url', 'title', 'toolbar=no, location=no, directories=no, status=no, menubar=no, scrollbars=no, resizable=no, copyhistory=no, width=100, height=200, top=20, left=20');
console.log('opened');
called = true;

</script>
</html>

[calvinmetcalf]

opened an issue with firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1202918

[defunctzombie]

So this ends up being a browser issue? Do we need to patch it? Seems like this check is harmless enough and code impact minimal.

On Tuesday, September 8, 2015, Calvin Metcalf notifications@github.com wrote:

opened an issue with firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1202918

— Reply to this email directly or view it on GitHub https://github.com/defunctzombie/node-process/pull/50#issuecomment-138734971 .

[calvinmetcalf]

may as well merge it, but yeah it seems to be a firefox bug

[defunctzombie]

thanks for the work folks! merged and released in v0.11.2