This worked locally, sent to tryserver for a full run

This is the companion patch to bug 673569. It makes the necessary changes to the framescripts to work with the new default of not sharing the scope. notes: - the only scripts that I'm keeping with global scope are the remote heads of the mobile mochitests. The whole purpose of the code there is to export useful functions, so blocking the shared scope and exporting one by one would be unecessary trouble - mobile/chrome/content/Util.js is an interesting case, as it is loaded both as a framescript in content, and as a <script> tag in the parent.. In the parent, Cc and Ci are already defined; in content they are not.. that's why I have to use `let` instead of `const`. The other alternative would be `if (!Cc) { ... }`

Hm we'll need to deploy this one line to talos before being able to land bug 673569 on m-c. Alice or jmaher: 673569 will change the way that framescripts are run, in that they won't all share the same scope. This means that objects from one script that are accessed in another one need to be explicitly exported through the `this` object now. (MozillaFileLogger is accessed in ipc.js)

I am having trouble making this work on android. I have a try server build with the patch to fennec loaded and the patch to talos and I am never able to generate a log file. I haven't seen any thing in my initial debugging, I will spend a bit more time on this later today.

(In reply to Joel Maher (:jmaher) from comment #4) > I am having trouble making this work on android. I have a try server build > with the patch to fennec loaded and the patch to talos and I am never able > to generate a log file. I haven't seen any thing in my initial debugging, I > will spend a bit more time on this later today. Any news on this?

I uploaded this to try. The previous patch fails in tabview tests, so I'm hoping to get rest of the frame script loaders fixed first and then figure out why tabbview is behaving badly. (The failing test does seem to do certain things randomly either sync or async.)

Comment on attachment 558662 [details] [diff] [review] Talos patch What is MozillaFileLogger.js? Where is this code? I couldn't find this using mxr.

Here is a link to mozillaFileLogger: http://hg.mozilla.org/build/talos/file/c6aba61c0f78/scripts/MozillaFileLogger.js

Comment on attachment 568972 [details] [diff] [review] use global scope for tabview This seems to fail allover :(

Can these review request be handled with some priority? This is blocking bug 673569, which is a major annoyance for pending e10s work.

Comment on attachment 558662 [details] [diff] [review] Talos patch Review of attachment 558662 [details] [diff] [review]: ----------------------------------------------------------------- I am unable to run talos after applying this patch as my previous comments say.

Comment on attachment 558431 [details] [diff] [review] Patch I'm not sure we need this anymore

This takes care of the Firefox frontend.

This makes the necessary changes to Metro. Jim, for some context, this bug is in support of bug 673569. It adds a new parameter to loadFrameScript. By default, content scripts are loaded as if they were surrounded by (function() {...})(). If you pass true for the extra parameter, then the old behavior will be used. Originally I had wanted to avoid even the possibility of preserving the old behavior, so I had to make a lot of changes to Metro. In Firefox, I think this led to better code. Metro has a lot more sharing between content scripts though, so it's dubious. If you want, I can just change all the loadFrameScript calls to pass the extra parameter. Then this patch won't be needed.

These are the changes for b2g. I had to go with changing loadFrameScript since nothing else I tried worked.

Comment on attachment 831927 [details] [diff] [review] frontend-changes Review of attachment 831927 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/content/content-sessionStore.js @@ +8,5 @@ > Services.console.logStringMessage("SessionStoreContent: " + msg); > } > > let Cu = Components.utils; > +let Ci = Components.interfaces; leaving as a follow-up, to independently test that it doesn't break anything else, but: with this bug now fixed I wanted to convert all the Cc,Ci definitions to be consts ::: testing/marionette/mach_commands.py @@ +112,5 @@ > def run_marionette_test(self, tests, address=None, testtype=None): > + #if self.substs.get('ENABLE_MARIONETTE') != '1': > + # print(MARIONETTE_DISABLED % ('marionette-test', > + # self.mozconfig['path'])) > + # return 1 is this supposed to be part of the patch?

(In reply to Bill McCloskey (:billm) from comment #14) > Created attachment 831936 [details] [diff] [review] > metro-changes > > This makes the necessary changes to Metro. Jim, for some context, this bug > is in support of bug 673569. It adds a new parameter to loadFrameScript. By > default, content scripts are loaded as if they were surrounded by > (function() {...})(). If you pass true for the extra parameter, then the old > behavior will be used. > > Originally I had wanted to avoid even the possibility of preserving the old > behavior, so I had to make a lot of changes to Metro. In Firefox, I think > this led to better code. Metro has a lot more sharing between content > scripts though, so it's dubious. If you want, I can just change all the > loadFrameScript calls to pass the extra parameter. Then this patch won't be > needed. Wondering what patches I need in order to take this patch for a spin.. a rollup that applies to mc tip of dependent work would be appreciated. I'd like to do a bit of manual testing.

Sorry Jim, I only just saw your comment. Here's a rolled up patch that applies on top of 24eb4aead1ba.

Comment on attachment 831936 [details] [diff] [review] metro-changes I've been running with this this morning and I haven't found any issues. I think mbrubeck should look this over as well though.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b8f25b9f1d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1375faef4ea1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd1596ffb9f

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5045e4af5807 because either this or bug 673569 caused b2g emulator mochitest-5 to time out in test_innerWidthHeight_script.html, https://tbpl.mozilla.org/php/getParsedLog.php?id=31017792&tree=Mozilla-Inbound

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/30a8127b9277 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc00eed50b7d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aa50b69994

Backed out for frequent B2G reftest timeouts. Please make sure this has full green Try run before pushing it again. https://hg.mozilla.org/integration/mozilla-inbound/rev/874788e2239c https://tbpl.mozilla.org/php/getParsedLog.php?id=31413972&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=31413760&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=31413784&tree=Mozilla-Inbound

It looks like the error is now: 18:06:32 INFO - System JS : ERROR chrome://reftest/content/reftest-content.js:132 - ReferenceError: setTimeout is not defined (which is different than the one before, and which is strangely nondeterministic). Does anyone know who is supposed to define setTimeout in reftest-content.js?

http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#21 -> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Timer.jsm#24

(In reply to Olli Pettay [:smaug] from comment #25) > http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest- > content.js#21 -> > http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Timer.jsm#24 Oh, OK. But then why would it fail? If setTimeout is added to the global, then it should always be there.

Hmm, maybe this has something to do with Kyle's patch to share globals between JSMs? It's only enabled on b2g, and the code added in bug 810987 seems like it might be triggered when doing the import of Timer.jsm.

Kyle, this is the bug where the Cu.import failure we talked about is happening. The related bug, which changes how frame scripts are loaded, is bug 673569. I pushed code to try that manually wraps the frame script in (function() {...})() to see if the failure still happens then. It's nondeterministic, so I'll try to do some retriggers. https://tbpl.mozilla.org/?tree=Try&rev=5ccc6d483990 However, I set ni? in case anything pops out at you based on manual inspection.

OK, it does indeed fail in the same way without my patch if I wrap reftest-content.js with (function() { ... })(). Can you provide any advice, Kyle?

Blake and I poked at this a bit with little success. Try flipping http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#733 to false and seeing if this happens with that turned off?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #30) > Blake and I poked at this a bit with little success. Try flipping > http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#733 to false > and seeing if this happens with that turned off? https://tbpl.mozilla.org/?tree=Try&rev=7781981918d5

I retriggered each test once and got no failures. So it looks like reuseGlobal is indeed the problem.

Actually, I just noticed this: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#356 As far as I can tell, the value of jsloader.reuseGlobal is completely ignored. On desktop, at least, we read the pref's value in mozJSComponentLoader::ReallyInit before prefs have actually been loaded. I suspect that the same thing happens on b2g. So I think the test I did is bogus. I don't know why it passed this time and not before. Maybe it's because I based on a different rev than before. I've been playing around with enabling global reuse on desktop and I get lots of failures. Some of them are probably legitimate name conflicts, but I think I also see stuff that resembles what's happening in this bug. I'm going to see if that leads anywhere.

I gave up on this and just loaded reftest-content.js in the global scope. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbeed56db621 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a15ec074e47 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4cb698555c

https://hg.mozilla.org/mozilla-central/rev/fbeed56db621 https://hg.mozilla.org/mozilla-central/rev/4a15ec074e47 https://hg.mozilla.org/mozilla-central/rev/ff4cb698555c