Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use a single global for all JSMs

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
ASSIGNED
2 years ago
2 days ago

People

(Reporter: billm, Assigned: mccr8)

Tracking

(Depends on: 2 bugs, Blocks: 4 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 7 obsolete attachments)

59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
(Reporter)

Description

2 years ago
When writing frontend code, it's frequently necessary to trade off modular design (using small, self-contained JSMs) with low memory usage (each JSM uses a separate compartment). This is very frustrating.

I just measured the number of JSMs we load to start up a browser and load a random web page. It's ~250 in the main process and ~90 in the content process. We've previously measured that each new compartment consumes ~75K of overhead [1]. So the memory isn't a huge issue, but it's significant. And it's going to become more important as we try to increase the number of content processes.

I think it would be great if we could go back to using a single compartment for all JSMs (or at least for all non-addon JSMs). I think the goal should be to have these boundaries:

zones are lifetime boundaries
compartments are security boundaries
JSMs are scope boundaries

One way to implement this is similar to what we do for content scripts and ExecuteInGlobalAndReturnScope. We would have a single global for all JSMs. Each JSM would have its own NonSyntacticVariablesObject below that on the scope chain. We would always run JSM code with |this| set to the NonSyntacticVariablesObject (this is different than how content scripts work, but it seems doable).

As I understand it, the main outcomes of this change would be:
- JSMs would still be completely isolated from each other in terms of what variables they see.
- Builtin properties on the global like Array would be shared. But since we don't do any monkeypatching in the frontend that I know of, this doesn't seem like it would be an issue.
- WrappedNatives would be shared across JSMs, so a QI in one JSM would be visible to other JSMs. That's sorta weird but it seems tolerable.
- Expandos would be shared across JSMs. (I don't actually remember if this is already true?) Similar to above, this seems like something we could deal with.
- Memory reporters would no longer be able to distinguish JSMs. Not sure how big a deal that is.
- We'd probably get worse JIT performance.

Luke, can you advise about whether this idea makes sense? The JIT issue seems like the biggest concern. The use of NonSyntacticVariablesObject is a little strange since, unlike with ExecuteInGlobalAndReturnScope, the NonSyntacticVariablesObject will always be parented to the same global. So it's basically a normal scope except that we stick unqualified variable references there. Maybe we could use a different kind of object that still has some of the same properties but isn't deoptimized so much?

[1] https://mail.mozilla.org/pipermail/firefox-dev/2015-April/002878.html
Flags: needinfo?(luke)
(Assignee)

Updated

2 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 1

2 years ago
> Memory reporters would no longer be able to distinguish JSMs. Not sure how big a deal that is.

I think the only context in which we end up discussing JSMs when looking at about:memory reports is marveling at how many there are.

Comment 2

2 years ago
Shouldn't we just wait for shu or whoever was working on it to finish ES6 modules and then just make JSMs (and indeed the current caller ExecuteInGlobalAndReturnScope) into modules in the ES6 sense?  Unless the timeframe on that is too long, of course.
(Reporter)

Comment 3

2 years ago
Jon, when do you thing ES6 modules will be implemented to the point where we can start using them in the browser?
Flags: needinfo?(jcoppeard)
(In reply to Bill McCloskey (:billm) from comment #3)
There are some issues to work out first so I'd say not for a couple of months at least.
Flags: needinfo?(jcoppeard)
(Reporter)

Comment 5

2 years ago
OK. This seems like something we could do sooner than that, so I think we should still look into it.

Comment 6

2 years ago
Yeah, using modules seems like the real goal state here.  Since modules also share the same global scope, I would guess that moving to ExecuteInGlobalAndReturnScope is a step in the right direction in flushing out all the other issues you listed.
Flags: needinfo?(luke)
So how is this different than the compartment sharing stuff that we're doing on b2g today?
(Reporter)

Comment 8

2 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (UTC+8 July 17-25, expect delays) from comment #7)
> So how is this different than the compartment sharing stuff that we're doing
> on b2g today?

I think the only difference is that unqualified vars (i.e., variables that are never declared) would go on the special scope object rather than the global.

I guess we would also need to do some special stuff for Cu.import and loadSubScript to make this work properly. The b2g thing uses GetOutermostEnclosingFunctionOfScriptedCaller. I guess we could do a similar thing to find the enclosing JSScript and then map that to its scope. I'd like to avoid doing the trick of compiling these as functions since seems kind of weird.
In general I am all for something similar but less hacky :D
(Reporter)

Comment 10

2 years ago
Shu, it looks like we now have the invariant that a StaticNonSyntacticScopeObjects represents either 0+ non-syntactic DynamicWithObjects or a single NonSyntacticVariablesObject.

Would it be okay to change this so that a StaticNonSyntacticScopeObjects can also represent 0+ DynamicWithObjects followed by a single NonSyntacticVariablesObject (where the variables object is closer to the global)?

I don't see any obvious places where we rely on it being "either but not both". I don't understand the debugger stuff though.
Flags: needinfo?(shu)
(In reply to Bill McCloskey (:billm) from comment #0)
> I just measured the number of JSMs we load to start up a browser and load a
> random web page. It's ~250 in the main process and ~90 in the content
> process. We've previously measured that each new compartment consumes ~75K
> of overhead [1]. So the memory isn't a huge issue, but it's significant. And
> it's going to become more important as we try to increase the number of
> content processes.

Have we made any progress in figuring out where that overhead is coming from? I'm not necessarily opposed to this, but I'd much rather reduce the memory footprint of a compartment to something more negligible, since that's a win that would translate to the web as well.

See bug 989373 comment 23. Nick, have we made any progress in our understanding here?
Flags: needinfo?(n.nethercote)
(In reply to Bill McCloskey (:billm) from comment #10)
> Shu, it looks like we now have the invariant that a
> StaticNonSyntacticScopeObjects represents either 0+ non-syntactic
> DynamicWithObjects or a single NonSyntacticVariablesObject.
> 
> Would it be okay to change this so that a StaticNonSyntacticScopeObjects can
> also represent 0+ DynamicWithObjects followed by a single
> NonSyntacticVariablesObject (where the variables object is closer to the
> global)?
> 
> I don't see any obvious places where we rely on it being "either but not
> both". I don't understand the debugger stuff though.

The Debugger doesn't depend on this invariant AFAIK. I think it's fine to change the invariant to 0+ non-syntactic DynamicWiths followed by 1? NonSyntacticVariablesObject.
Flags: needinfo?(shu)
> See bug 989373 comment 23. Nick, have we made any progress in our
> understanding here?

No. Right now my level of understanding is basically "lots of small things that add up", and I don't even understand what all of those small things are.
Flags: needinfo?(n.nethercote)
(Reporter)

Comment 14

2 years ago
Created attachment 8639612 [details] [diff] [review]
patch

Here's a WIP patch. It seems to work pretty well, but I need to fix a few things and I'm not sure when I'll get a chance.
(Assignee)

Comment 15

2 years ago
I talked to billm about this, and he said in his local measurements it saved 15-20MB (but not too much in content processes).
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
status-firefox42: affected → ---
(Reporter)

Comment 16

2 years ago
Created attachment 8660182 [details] [diff] [review]
patch v2

This is a more recent version of the patch. It's based on an m-c revision from July 22 in case it helps in rebasing. I'll comment with some explanations.
Attachment #8639612 - Attachment is obsolete: true
(Reporter)

Comment 17

2 years ago
Comment on attachment 8660182 [details] [diff] [review]
patch v2

Review of attachment 8660182 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/lib/sdk/base64.js
@@ +14,5 @@
>  // (devtools loader injects these symbols as global and prevent using
>  // const here)
> +let scope = Cu.import("resource://gre/modules/Services.jsm", {});
> +let global = Cu.getGlobalForObject(scope);
> +let { atob, btoa } = global;

atob/btoa are not defined by Services.jsm. They're included on every BackstagePass, though. Even though they're not part of EXPORTED_SYMBOLS, Cu.import returns the full JSM global and so we can access them here.

::: addon-sdk/source/lib/toolkit/loader.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  ;((factory) => { // Module boilerplate :(
> +  let Cu = Components ? Components["utils"] : null;

This isn't needed now (see later about loader.js).

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +10,5 @@
>  let Cr = Components.results;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  Cu.import("resource://gre/modules/Timer.jsm", this);
> +Cu.import("resource://gre/modules/Services.jsm", this);

Lots of frame scripts assume that stuff imported by Cu.import goes on the shared frame script global, so they rely on other scripts to Cu.import stuff for them. This patch makes Cu.import add stuff to the module scope, so the sharing no longer works.

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +613,5 @@
>    }
>  
>    RangedChromeActions.prototype = Object.create(ChromeActions.prototype);
>    var proto = RangedChromeActions.prototype;
> +  Object.defineProperty(proto, "constructor", {value: RangedChromeActions});

The changes in this file probably are no longer needed.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +492,5 @@
> +    bool reuseGlobal = !aAddonId;
> +
> +    nsCString spec;
> +    aURI->GetSpec(spec);
> +    if (spec.EqualsASCII("resource://gre/modules/commonjs/toolkit/loader.js"))

These cutouts are the worst part of this patch. We might be able to add some kind of API so we can do this from JS instead of C++, but it's still bad.

The loader.js is tricky because it calls Object.freeze on things like Object and Object.prototype:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#92
With this patch, all JSMs share the same copies of Object, Array, etc.

We don't do any monkeypatching in Firefox as far as I know, so this isn't bad in itself. The problem happens for this reason. Let's say you want to set foo.constructor = bar where foo is some prototype object. Normally that works fine and adds a property directly on foo. However, foo's proto is Object.prototype, which also has a constructor property. This property is now frozen. For some reason, ES semantics say that if foo's proto has a frozen property, you can't set that property on foo, even if foo itself is not frozen.

I started to fix this by converting assignments to such properties to Object.defineProperty instead, since this has its own rules. However, I eventually decided that this was really gross and loader.js should not be freezing shared stuff. So I decided to move it out of the shared global.

@@ +494,5 @@
> +    nsCString spec;
> +    aURI->GetSpec(spec);
> +    if (spec.EqualsASCII("resource://gre/modules/commonjs/toolkit/loader.js"))
> +        reuseGlobal = false;
> +    else if (spec.EqualsASCII("resource://gre/modules/jsdebugger.jsm"))

This is necessary because of the following common pattern:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js#22

We have a function that adds the builtin Debugger property to a global. We load a JSM and then try to add Debugger to that global. With this patch, the JSM is no longer a global, so we can't add Debugger to it.

This is all for test case, so I just got frustrated and made jsdebugger.jsm be its own global. Probably a better thing to do here is to convert such tests to call a function that makes a Sandbox global and adds Debugger to that instead.

::: toolkit/components/search/nsSearchService.js
@@ -220,5 @@
>  function isUsefulLine(aLine) {
>    return !(/^\s*($|#)/i.test(aLine));
>  }
>  
> -this.__defineGetter__("FileUtils", function() {

BackstagePass has __defineGetter__ on it but the NonSyntacticVariablesObject doesn't. It's deprecated anyway, so I replaced them all with Object.defineProperty.

Comment 18

10 months ago
Bill, what's the status of this?  It sounds like it might be a good step along the way to replacing JSM modules with ES modules.
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 19

10 months ago
This was mostly working at the time, but it was pretty gross. There are just some horrible hacks that some code does and it was difficult to work around them. I think a more incremental approach, like the one proposed in bug 1305669, would be better. Some of the code in the patch here might be useful as a starting point, but landing it as-is wouldn't necessarily help much with bug 1305669.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

4 months ago
Blocks: 1353816
(Assignee)

Updated

3 months ago
Depends on: 1356666
(Assignee)

Updated

3 months ago
Depends on: 1356799
(Assignee)

Comment 20

3 months ago
I've started rebasing this patch. I'm mostly just trying to figure out how the scope and environment stuff needs to be changed in light of the rewrite Shu did recently.
(Assignee)

Updated

3 months ago
Depends on: 1357828
(Assignee)

Updated

3 months ago
See Also: → bug 1338907
(Assignee)

Comment 21

3 months ago
Created attachment 8861197 [details] [diff] [review]
Use a single global for all JSMs. WIP
(Assignee)

Comment 22

3 months ago
The current problem I need to fix is that if |this| is not explicitly defined for a function, it should use the NonSyntacticVariablesObject instead of the global. This seems to happen in js::BoxNonStrictThis().
(Assignee)

Comment 23

3 months ago
I managed to get this at least sort of working (I'm able to start up the browser without any error messages). The memory win is less than I was seeing before with the b2g merging: around 720kb saved in the content process, and 16mb saved in the parent process. The content process savings isn't too impressive, but the parent process amount is pretty good.

I suspect I may have been seeing more with the b2g merging because scripts weren't fixed up properly, so things throw a lot of errors, and so we simply don't load as many scripts. I hacked up some patches to fix up the b2g merging a little in the content process, and the savings were about what I'm seeing for this approach.
(Assignee)

Comment 24

3 months ago
Created attachment 8863521 [details] [diff] [review]
Use a single global for all JSMs. WIP
(Assignee)

Updated

3 months ago
Attachment #8861197 - Attachment is obsolete: true
(Assignee)

Comment 25

3 months ago
Created attachment 8863878 [details]
memory report with one tab and a shared JSM global
(Assignee)

Comment 26

3 months ago
I figured out part of why global merging does not improve memory usage by much in the content process. Bug 1303754 changed setSourceIsLazy to false in the content process, which enables lazy parsing (when we can do that is determined by BytecodeCompiler::canLazilyParse()), which Jan found saved 1mb+ in the child process. This patch adds a non-syntactic variables object onto the scope chain, which causes canLazilyParse() to return false, so we lose out on the 1mb. Presumably the actual memory gain is like 1.3MB or so, but much of that is cancelled out. The parent process doesn't use lazy parsing, so it is all win.

The parent process has 6 times as many compartments, so if it is saving 16mb, we might expect to save 2.6mb in the child process. I'm not sure if there's another 1mb we should be saving somewhere.
(Assignee)

Updated

3 months ago
Depends on: 1363215
(Assignee)

Updated

2 months ago
Depends on: 1363443
(Assignee)

Comment 27

2 months ago
On advice of Shu, I just removed the line from canLazilyParse() for non syntactic scopes, and it at least works well enough to load a simple page without any noticable errors. With that, content process memory usage was reduced by 2.75mb, which is in line with what I'd expect.
(Assignee)

Updated

2 months ago
Depends on: 1364566
(Assignee)

Updated

2 months ago
Depends on: 989373
(Assignee)

Updated

2 months ago
Depends on: 1365767
(Assignee)

Updated

2 months ago
Depends on: 1366023
(Assignee)

Comment 28

2 months ago
Created attachment 8869209 [details] [diff] [review]
Use a single global for all JSMs. WIP.

I split out the removal of the old B2G merging code into bug 989373. This is rebased on top of that.

I've also split out some of the frontend fixes that are needed, but they aren't all here yet.
(Assignee)

Updated

2 months ago
Attachment #8863521 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
See Also: → bug 1357862
(Assignee)

Updated

2 months ago
Depends on: 1366896
(Assignee)

Updated

2 months ago
Depends on: 1368170
(Assignee)

Updated

2 months ago
Depends on: 1368195
Just to add another data point for this:

Since bug 1364974, we pre-compile all module scripts into xpc::CompilationScope, and then clone them into global of new module when it's created. This winds up being much faster than XDR decoding, but still much slower than not having to clone at all. If we're loading all modules into the same global, we can compile for the shared module global instead, and avoid the clone altogether (as long as we either don't need to compile for non-syntactic scopes, or initially compile the scripts for non-syntactic scopes rather than global scopes).

Also, the script preloader patches disabled lazy parsing and enabled lazy source in both parent and child processes, so if we were previously saving any memory from that, it should already no longer be the case.

But my medium/long-term thoughts were that, because we share the original mmapped XDR data for pre-loaded module scripts across all processes, we should be able to reuse a slice of that original data whenever we re-lazify module functions, which means that there wouldn't be any significant per-process overhead for functions which aren't used very often.
(Assignee)

Comment 30

2 months ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #29)
> Also, the script preloader patches disabled lazy parsing and enabled lazy
> source in both parent and child processes, so if we were previously saving
> any memory from that, it should already no longer be the case.

Hmm that seems bad. If that was in the patches I reviewed, I guess I didn't noice it. It was saving a few megs per content process (it was already disabled in the parent process). Kind of odd it wouldn't show up on AWSY.
(In reply to Andrew McCreight [:mccr8] from comment #30)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment
> #29)
> > Also, the script preloader patches disabled lazy parsing and enabled lazy
> > source in both parent and child processes, so if we were previously saving
> > any memory from that, it should already no longer be the case.
> 
> Hmm that seems bad. If that was in the patches I reviewed, I guess I didn't
> noice it. It was saving a few megs per content process (it was already
> disabled in the parent process). Kind of odd it wouldn't show up on AWSY.

It was disabled as a side-effect of caching being enabled, since we enable lazy source only when we're not using the cache.

Given that we didn't see any AWSY regressions, something may have changed in the mean time, or we may see smaller memory wins from lazy parsing when we're using the bytecode cache.
(Assignee)

Updated

2 months ago
Depends on: 1369214
(Assignee)

Comment 32

2 months ago
I've been fixing test failures over the last week or two. I think it should be green now, at least on Linux. I haven't tried other platforms. Most of the changes were tweaks to how JSMs were being imported and the like, which mostly required running the test and looking for JS error messages, but two issues were more complex to investigate.

My code for evaluating JSOP_FUNCTIONTHIS |this| in a non-syntactic scope was returning the |this| from the innermost dynamic lexical environment, rather than from the NSVO. This was breaking an XPConnect test, test_bug993423.html, which runs an inner function inside an XBL event handler to get the value of |this|. I should write some more explicit tests for this issue, as it is mostly luck that a test existed that caught this issue. My current fix also feels a little hacky.

test_exceptionSanitization.html (and another similar DOM bindings test) was failing, but only with e10s. This test checks that an exception thrown by chrome code to content gets caught and replaced with a clean new exception. It turns out that there's 
a method Cu.forcePermissiveCOWs() that testing code can use to basically disable security wrappers. This works by setting a flag on the compartment, and gets loaded in a couple of testing JSMs. If those testing JSMs are merged in with the rest, then these wrappers are disabled everywhere, and the test fails. This only happens with e10s because I think these JSMs are only loaded for e10s. This is easily fixed by blacklisting those JSMs from being merged, but I should also add an assertion that we never set this testing flag on the shared JSM global. Bill suggested that I add some similar asserts for other things that should never get merged into the shared global.

I did an AWSY run, which showed a memory improvement of about 10MB.

This is all without lazy parsing. I need to investigate what happens with that next, both in terms of test failures and memory usage. The caching stuff kmag added recently may affect what happens in terms of memory.
Andrew, this is great to see!

I was curious, have you tried profiling these builds?  One of the things that I'm hoping this change to achieve is eliminating cross-compartment wrappers between the objects imported from JSMs.  It would be great to try to profile some chrome-JS heavy test with this change to see if it has any measurable impact on performance too.  mconley and florian can probably help think of a good test case.  I'm thinking maybe startup on the reference hardware?
(Assignee)

Comment 34

2 months ago
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #33)
> One of the things
> that I'm hoping this change to achieve is eliminating cross-compartment
> wrappers between the objects imported from JSMs.

Yes, it should do that. I'm not sure how much of an impact this will have. Chrome JS doesn't even get Ion compiled most of the time so I'm not sure how much time is spent in CCWs. This patch may actually worsen perf by preventing Ion compilation entirely. But yes, I should see what affect there is.
I know it's anecdotal but while profiling the new l10n API we're preparing for post Quantum landing, :smaug pointed out today that we are doing some cross compartment back and forth since we introduce 4 jsms.
I can bundle them into one but that would reduce the code maintainability so I hope for this bug to remove the issue for me :)

On a related note - is there a way to check if my jsm jits?
(Assignee)

Updated

2 months ago
Depends on: 1370732
(Assignee)

Updated

2 months ago
Depends on: 1370733
(Assignee)

Updated

a month ago
Depends on: 1371042
(Assignee)

Comment 36

a month ago
I don't know how much I believe it, but according to a try run I did, this makes tp5o responsiveness opt e10s more than 30% better. ;) I don't know if that is meaningful. tp5o opt e10s was flat. AWSY was about 10mb better, as before, though this time I had lazy parsing enabled for nonsyntactic scopes.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a49112c7a576&newProject=try&newRevision=9f93aef69dfe5f7ceb52d92ffad0595cffc98507&framework=1&showOnlyImportant=0
(In reply to Andrew McCreight [:mccr8] from comment #36)
> I don't know how much I believe it, but according to a try run I did, this
> makes tp5o responsiveness opt e10s more than 30% better. ;) I don't know if
> that is meaningful.

Given the number of stacks I've seen with multiple JIT fallbacks triggered by cross-compartment wrapper access, it wouldn't entirely surprise me. Assuming that the use of non-syntactic scopes doesn't completely destroy the JITtability of most of our chrome code, anyway.
(Assignee)

Comment 38

a month ago
In some local testing, main process memory went down by a little over 8mb, and a single child process went down by about 4.7mb, so for 4 child processes I'd expect a bit more of a reduction, but I'm not sure what the discrepancy is.
(Assignee)

Updated

a month ago
Depends on: 1371844
(Assignee)

Updated

a month ago
Depends on: 1372295

Updated

a month ago
See Also: → bug 1373418
(Assignee)

Updated

a month ago
See Also: → bug 1374716
(Assignee)

Updated

a month ago
Depends on: 1375133
(Assignee)

Updated

a month ago
Depends on: 1375188
(Assignee)

Updated

a month ago
Depends on: 1375262
(Assignee)

Comment 39

29 days ago
Created attachment 8880957 [details] [diff] [review]
share JSM globals, WIP
Attachment #8863878 - Attachment is obsolete: true
Attachment #8869209 - Attachment is obsolete: true
(Assignee)

Updated

29 days ago
Assignee: wmccloskey → continuation
(Assignee)

Updated

16 days ago
Depends on: 1379023
(Assignee)

Updated

15 days ago
Attachment #8660182 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8880957 - Attachment is obsolete: true
(Assignee)

Updated

4 days ago
Depends on: 1381919
(Assignee)

Updated

4 days ago
Blocks: 1381961
(Assignee)

Updated

4 days ago
Blocks: 1381965
Blocks: 1381976
You need to log in before you can comment on or make changes to this bug.