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

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: smaug, Assigned: billm)

Tracking

unspecified
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments, 2 obsolete attachments)

No description provided.
Assignee: nobody → felipc
Hardware: x86_64 → All
Posted patch WIP (obsolete) — Splinter Review
This worked locally, sent to tryserver for a full run
Posted patch PatchSplinter Review
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) { ... }`
Attachment #558008 - Attachment is obsolete: true
Attachment #558431 - Flags: review?(mark.finkle)
Posted patch Talos patchSplinter Review
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)
Attachment #558662 - Flags: review?(jmaher)
Attachment #558662 - Flags: review?(anodelman)
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?
Posted patch use global scope for tabview (obsolete) — Splinter Review
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.
Comment on attachment 568972 [details] [diff] [review]
use global scope for tabview

This seems to fail allover :(
Attachment #568972 - Attachment is obsolete: true
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.
Attachment #558662 - Flags: review?(jmaher) → review-
Comment on attachment 558431 [details] [diff] [review]
Patch

I'm not sure we need this anymore
Attachment #558431 - Flags: review?(mark.finkle)
This takes care of the Firefox frontend.
Attachment #831927 - Flags: review?(felipc)
Posted patch metro-changesSplinter Review
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.
Attachment #831936 - Flags: review?(jmathies)
Posted patch b2g-changesSplinter Review
These are the changes for b2g. I had to go with changing loadFrameScript since nothing else I tried worked.
Attachment #831937 - Flags: review?(fabrice)
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?
Attachment #831927 - Flags: review?(felipc) → review+
Attachment #831937 - Flags: review?(fabrice) → review+
(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.
Posted patch rolled-upSplinter Review
Sorry Jim, I only just saw your comment. Here's a rolled up patch that applies on top of 24eb4aead1ba.
Attachment #8334944 - Flags: feedback?(jmathies)
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.
Attachment #831936 - Flags: review?(mbrubeck)
Attachment #831936 - Flags: review?(jmathies)
Attachment #831936 - Flags: review+
Attachment #831936 - Flags: review?(mbrubeck) → review+
Attachment #8334944 - Flags: feedback?(jmathies)
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
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?
(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.
Flags: needinfo?(khuey)
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?
Flags: needinfo?(khuey) → needinfo?(wmccloskey)
(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
Flags: needinfo?(wmccloskey)
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.
Whiteboard: [qa-]
Attachment #558662 - Flags: review?(anodelman)
You need to log in before you can comment on or make changes to this bug.