682048 - Change frame script handling to support anon/global scope (see Bug 673569 )

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP

This worked locally, sent to tryserver for a full run

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

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) { ... }`

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Talos patch

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)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: use global scope for tabview

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 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 558662 [details] [diff] [review]
Talos patch

What is MozillaFileLogger.js? Where is this code? I couldn't find this using mxr.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 568972 [details] [diff] [review]
use global scope for tabview

This seems to fail allover :(

Comment 10

[Dão Gottwald [:dao]]

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

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

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 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 558431 [details] [diff] [review]
Patch

I'm not sure we need this anymore

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: frontend-changes

This takes care of the Firefox frontend.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: b2g-changes

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

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 17

[Jim Mathies [:jimm]]

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

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rolled-up

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

Comment 19

[Jim Mathies [:jimm]]

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.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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