Resolve late injection race conditional

[sizzlemctwizzle]

While dependencies are being downloaded store the windows that wish to inject the script. Once the downloads have completed, late inject the script in all those windows.

Ref: http://groups.google.com/group/greasemonkey-dev/browse_thread/thread/6025568018a7108d/497343594244f2bd

[sizzlemctwizzle]

The patch for this has been completed.

[arantius]

Really liking that patch! I'd just prefer a better name than script.wins. Something about pending execution? Just "win" is too generic.

[sizzlemctwizzle]

Alright, I changed it to something less vague.

[sizzlemctwizzle]

I've discovered a few more issue. I'll need a little while longer to resolve them.

[sizzlemctwizzle]

Okay, the branch now fixes all late injection failure issues. I ran into tons of gotchas, but now everything appears to be stable.

[arantius]

Thanks a million for the work! I was pretty sure there were issues buried in here, but hadn't got to looking into it deeply yet.

[arantius]

Quick review:

• config.js line 334 -- braces around multi-line if (or collapse to one line)

• do we still need the delayInjection flag, or does pendingExec != null serve that purpose now?

• initialize pendingExec to [] in the constructor, then scriptdownloader.js line 209 can be something slightly simpler like:

  while (var pendingExec = this.script.pendingExec.shift()) {
  GM_getConfig().injectScript(this.script, pendingExec);
  }

And (how) are we handling the case where the window doesn't exist anymore, once the dependencies have been downloaded? Does it still exist, because of the pendingExec reference, and running the script works, but basically accomplishes nothing? Are we leaking memory with these window references (i.e. does running the script create another reference)?

[sizzlemctwizzle]

Thanks a million for the work!

You're very welcome.

I was pretty sure there were issues buried in here

Yeah I thought so too. This issue has been lingering even since I started live meta and want this feature(re-downloading dependencies) to work intuitively for users. Hopefully I've finally managed to do that.

Quick review:

Okay, most of your suggestions are in this commit. Although I didn't set pendingExec to [] in the constructor because we don't need it to use that loop syntax, plus if its not set to null when unused it can't be used as a flag.

And (how) are we handling the case where the window doesn't exist anymore

The reference to a closed window only exists temporarily until we finish downloading the scripts. Then it gets thrown away with the other windows.

Does it still exist, because of the pendingExec reference

The way I understand it all we have is a handle to the window, we can't actually keep the window around by keeping the handle to it.

Are we leaking memory with these window references (i.e. does running the script create another reference)?

In JavaScript the only way to leak memory is to hold on to a reference. By setting this.script.pendingExec to null on line 209 of scriptdownloader.js we ditch our only reference to the windows so the GC can reclaim the memory. All other references to the windows are local so they go out of scope and can be reclaimed.

[sizzlemctwizzle]

I've merged this branch with the latest upstream changes.

[arantius]

Fixed by fast-forward merge (of about a dozen commits), so there's nothing obvious to link to. But done!