Closed Bug 888338 Opened 6 years ago Closed 6 years ago

GC: Post-barrier more browser heap based GC thing pointers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(7 files, 4 obsolete files)

984 bytes, patch
terrence
: review+
Details | Diff | Splinter Review
7.52 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.97 KB, patch
bholley
: review+
Details | Diff | Splinter Review
15.25 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
8.42 KB, patch
jonco
: review+
Details | Diff | Splinter Review
11.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
31.57 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Searching for JS_Call*Tracer reveals there are a bunch of places where heap things are traced that we haven't converted to use post-barriers.

The main areas are:
 - workers
 - XPConnect
 - bindings
Hmm.  Where do you see an issue in bindings code here?
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #1)
> Hmm.  Where do you see an issue in bindings code here?

For example, in WebComponentsBinding.cpp, ElementRegistrationOptions::TraceDictionary() calls JS_CallObjectTracer() to trace its mPrototype member.

Digging a little further, it seems TraceDictionary() is only called for stack rooting - are these classes never used on the heap then?
TraceDictionary is indeed only called for stack rooting.  The dictionary itself might be on the heap, but only as part of a data structure that has stack lifetime and stack rooting.

Sadly, we have no way to statically enforce that.  :(
Attachment #771425 - Flags: review?(terrence)
Attachment #771426 - Flags: review?(terrence)
Attachment #771428 - Flags: review?(terrence)
Attachment #771429 - Flags: review?(sphink)
Attachment #771430 - Flags: review?(bobbyholley+bmo)
Attachment #771431 - Flags: review?(bzbarsky)
Attached patch 7 - barriers in workers (obsolete) — Splinter Review
Attachment #771433 - Flags: review?(bzbarsky)
Olli or I are probably better reviewers for 6 and 7 as bz is on paternity leave.
(In reply to Andrew McCreight [:mccr8] from comment #11)

Thanks, I'll update.
Attachment #771431 - Flags: review?(bzbarsky) → review?(continuation)
Attachment #771433 - Flags: review?(bzbarsky) → review?(bugs)
Comment on attachment 771426 [details] [diff] [review]
2 - give post barrier callback function an extra data argument

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

r=me

::: js/src/gc/StoreBuffer.h
@@ +60,5 @@
>              return;
>          ValueType value = p->value;
>          JS_SET_TRACING_LOCATION(trc, (void*)&p->key);
>          Mark(trc, &key, "HashKeyRef");
> +        map->rekey(prior, key);

I've got a similar hunk in bug 888117. If you land first, also remove value above, and the definition of ValueType and Ptr, and switch &p->key to &*p so that this class can be used with HashSets too.
Attachment #771426 - Flags: review?(terrence) → review+
Comment on attachment 771428 [details] [diff] [review]
3 - don't call gray root tracer for minor GCs

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

r=me
Attachment #771428 - Flags: review?(terrence) → review+
Comment on attachment 771425 [details] [diff] [review]
1 - add trace method for objects we know are tenured

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

Instead of scattering assertions all over the place and adding a new API, could we instead make a new type TenuredHeap on <typename T, size_t Mask=0> that overrides the constructor and operator= to make the assertion? This would also allow us to override JS_CallHeapObjectTracer for the assertion at mark time and, I think, remove CallMaskedObjectTracer too. The only complexity is the Mask parameter to the template, but that shouldn't be a serious obstacle.
Attachment #771425 - Flags: review?(terrence)
Comment on attachment 771430 [details] [diff] [review]
5 - a couple more barriers in XPConnect

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

I don't really know what this stuff does, but it looks reasonable.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +3407,5 @@
>      RemoveFromRootSet(nsXPConnect::GetRuntimeInstance()->GetMapLock());
>  }
>  
>  void
>  XPCJSObjectHolder::TraceJS(JSTracer *trc)

This is occasionally used on the stack, I believe. That should be ok, right?
Attachment #771430 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 771433 [details] [diff] [review]
7 - barriers in workers

>-  JS::Value mTimeoutVal;
>-  nsTArray<jsval> mExtraArgVals;
>+  JS::Heap<JS::Value> mTimeoutVal;
>+  nsTArray<JS::Heap<jsval> > mExtraArgVals;
While you're here, change jsval to JS::Value

>       if (!JS_CallFunctionValue(aCx, global, info->mTimeoutVal,
>                                 info->mExtraArgVals.Length(),
>-                                info->mExtraArgVals.Elements(), rval.address()) &&
>+                                info->mExtraArgVals.Elements()->unsafeGet(),
>+                                rval.address()) &&
Perhaps add a comment why unsafeGet is needed.

>     XMLHttpRequest::StateData state;
>-    // XXXbz there is no AutoValueRooter anymore?
>-    JS::AutoArrayRooter rooter(aCx, 1, &state.mResponse);
>+    JS::Rooted<JS::Value> response(aCx);
> 
>     state.mResponseTextResult = mResponseTextResult;
>     state.mResponseText = mResponseText;
> 
>     if (NS_SUCCEEDED(mResponseTextResult)) {
>       MOZ_ASSERT(JSVAL_IS_VOID(mResponse) || JSVAL_IS_NULL(mResponse));
>       state.mResponseResult = mResponseTextResult;
>-      state.mResponse = mResponse;
>+      response = mResponse;
>     }
>     else {
>       state.mResponseResult = mResponseResult;
> 
>       if (NS_SUCCEEDED(mResponseResult)) {
>         if (mResponseBuffer.data()) {
>           MOZ_ASSERT(JSVAL_IS_VOID(mResponse));
> 
>@@ -689,38 +688,35 @@ public:
>           JSStructuredCloneCallbacks* callbacks =
>             aWorkerPrivate->IsChromeWorker() ?
>             ChromeWorkerStructuredCloneCallbacks(false) :
>             WorkerStructuredCloneCallbacks(false);
> 
>           nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
>           clonedObjects.SwapElements(mClonedObjects);
> 
>-          JS::Rooted<JS::Value> response(aCx);
>           if (!responseBuffer.read(aCx, response.address(), callbacks, &clonedObjects)) {
>             return false;
>           }
>-
>-          state.mResponse = response;
>         }
>         else {
>-          state.mResponse = mResponse;
>+          response = mResponse;
>         }
>       }
>     }
> 
>     state.mStatusResult = mStatusResult;
>     state.mStatus = mStatus;
> 
>     state.mStatusText = mStatusText;
> 
>     state.mReadyState = mReadyState;
> 
>     XMLHttpRequest* xhr = mProxy->mXMLHttpRequestPrivate;
>-    xhr->UpdateState(state);
>+    xhr->UpdateState(state, response);
> 
>     if (mUploadEvent && !xhr->GetUploadObjectNoCreate()) {
>       return true;
>     }
> 
>     JS::Rooted<JSString*> type(aCx, JS_NewUCStringCopyN(aCx, mType.get(), mType.Length()));
>     if (!type) {
>       return false;
>@@ -1401,36 +1397,36 @@ Proxy::HandleEvent(nsIDOMEvent* aEvent)
>   }
> 
>   return NS_OK;
> }
> 
> XMLHttpRequest::XMLHttpRequest(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> : XMLHttpRequestEventTarget(aCx), mJSObject(NULL), mUpload(NULL),
>   mWorkerPrivate(aWorkerPrivate),
>-  mResponseType(XMLHttpRequestResponseType::Text), mTimeout(0),
>-  mJSObjectRooted(false), mBackgroundRequest(false),
>+  mResponseType(XMLHttpRequestResponseType::Text), mResponse(JSVAL_VOID), mTimeout(0),
>+  mJSObjectRooted(nullptr), mBackgroundRequest(false),
>   mWithCredentials(false), mCanceled(false), mMozAnon(false), mMozSystem(false)
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
> }
> 
> XMLHttpRequest::~XMLHttpRequest()
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
>   MOZ_ASSERT(!mJSObjectRooted);
> }
> 
> void
> XMLHttpRequest::_trace(JSTracer* aTrc)
> {
>   if (mUpload) {
>-    mUpload->TraceJSObject(aTrc, "mUpload");
>+    mUpload->TraceJSObject(aTrc, "XMLHttpRequest::mUpload");
>   }
>-  JS_CallValueTracer(aTrc, &mStateData.mResponse, "mResponse");
>+  JS_CallHeapValueTracer(aTrc, &mResponse, "XMLHttpRequest::mResponse");
>   XMLHttpRequestEventTarget::_trace(aTrc);
> }
> 
> void
> XMLHttpRequest::_finalize(JSFreeOp* aFop)
> {
>   ReleaseProxy(XHRIsGoingAway);
>   XMLHttpRequestEventTarget::_finalize(aFop);
>@@ -1505,28 +1501,29 @@ XMLHttpRequest::MaybePin(ErrorResult& aR
>   mWorkerPrivate->AssertIsOnWorkerThread();
> 
>   if (mJSObjectRooted) {
>     return;
>   }
> 
>   JSContext* cx = GetJSContext();
> 
>-  if (!JS_AddNamedObjectRoot(cx, &mJSObject, "XMLHttpRequest mJSObject")) {
>+  mJSObjectRooted = mJSObject;
>+  if (!JS_AddNamedObjectRoot(cx, &mJSObjectRooted, "XMLHttpRequest::mJSObjectRooted")) {
>+    mJSObjectRooted = nullptr;
>     aRv.Throw(NS_ERROR_FAILURE);
>     return;
>   }
> 
>   if (!mWorkerPrivate->AddFeature(cx, this)) {
>-    JS_RemoveObjectRoot(cx, &mJSObject);
>+    JS_RemoveObjectRoot(cx, &mJSObjectRooted);
>+    mJSObjectRooted = nullptr;
>     aRv.Throw(NS_ERROR_FAILURE);
>     return;
>   }
>-
>-  mJSObjectRooted = true;
> }
> 
> void
> XMLHttpRequest::MaybeDispatchPrematureAbortEvents(ErrorResult& aRv)
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
>   MOZ_ASSERT(mProxy);
> 
>@@ -1630,21 +1627,21 @@ void
> XMLHttpRequest::Unpin()
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
> 
>   NS_ASSERTION(mJSObjectRooted, "Mismatched calls to Unpin!");
> 
>   JSContext* cx = GetJSContext();
> 
>-  JS_RemoveObjectRoot(cx, &mJSObject);
>+  JS_RemoveObjectRoot(cx, &mJSObjectRooted);
> 
>   mWorkerPrivate->RemoveFeature(cx, this);
> 
>-  mJSObjectRooted = false;
>+  mJSObjectRooted = nullptr;
> }
> 
> void
> XMLHttpRequest::SendInternal(const nsAString& aStringBody,
>                              JSAutoStructuredCloneBuffer& aBody,
>                              nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects,
>                              ErrorResult& aRv)
> {
>@@ -2126,34 +2123,33 @@ XMLHttpRequest::SetResponseType(XMLHttpR
>   runnable->GetResponseType(acceptedResponseTypeString);
> 
>   mResponseType = ConvertStringToResponseType(acceptedResponseTypeString);
> }
> 
> jsval
> XMLHttpRequest::GetResponse(JSContext* /* unused */, ErrorResult& aRv)
> {
>-  if (NS_SUCCEEDED(mStateData.mResponseTextResult) &&
>-      JSVAL_IS_VOID(mStateData.mResponse)) {
>+  if (NS_SUCCEEDED(mStateData.mResponseTextResult) && JSVAL_IS_VOID(mResponse)) {
>     MOZ_ASSERT(mStateData.mResponseText.Length());
>     MOZ_ASSERT(NS_SUCCEEDED(mStateData.mResponseResult));
> 
>     JSString* str =
>       JS_NewUCStringCopyN(GetJSContext(), mStateData.mResponseText.get(),
>                           mStateData.mResponseText.Length());
>     if (!str) {
>       aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>       return JSVAL_VOID;
>     }
> 
>-    mStateData.mResponse = STRING_TO_JSVAL(str);
>+    mResponse = STRING_TO_JSVAL(str);
>   }
> 
>   aRv = mStateData.mResponseResult;
>-  return mStateData.mResponse;
>+  return mResponse;
> }

>-  bool mJSObjectRooted;
>+  JSObject* mJSObjectRooted;
So this should be now mRootedJSObject


>   void
>-  UpdateState(const StateData& aStateData)
>+  UpdateState(const StateData& aStateData, JS::Handle<JS::Value> aResponse)
>   {
>     mStateData = aStateData;
>+    mResponse = aResponse;
>   }
This is odd, since the method starts to do something else than just UpdateState.
The whole statedata is a bit odd, but could you keep all the mResponse* stuff there.
Attachment #771433 - Flags: review?(bugs) → review-
Comment on attachment 771431 [details] [diff] [review]
6 - barriers in the browser

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

::: dom/bindings/BindingUtils.h
@@ +273,5 @@
>  {
>    MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL);
>    MOZ_ASSERT(js::GetReservedSlot(obj, DOM_PROTOTYPE_SLOT).isUndefined());
>  
> +  JS::Heap<JSObject*>* protoAndIfaceArray = new JS::Heap<JSObject*>[kProtoAndIfaceCacheCount];

I guess JS::Heap always initializes to zero?
Attachment #771431 - Flags: review?(continuation) → review+
Attached patch 1 - Add TenuredHeap<T> class (obsolete) — Splinter Review
As per comments, I added a TenuredHeap<T> class for when we know the thing is always in the tenured heap.

This is better overall, but we still have to have the assertion for a couple of
cases where we need to trace a global that we've just got from calling the JS
API.

I hardcoded the number of mask bits because otherwise you end up with a
different type for each number of bits and have to overload the trace function
accordingly.
Attachment #771425 - Attachment is obsolete: true
Attachment #772698 - Flags: review?(terrence)
Attachment #771426 - Attachment is obsolete: true
Updated following review comments.
Attachment #771433 - Attachment is obsolete: true
Attachment #772701 - Flags: review?(bugs)
Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

This touches some pretty central stuff in nsGlobalWindow and XPCWNs so somebody like maybe peterv or bz should review this part...
Attachment #772698 - Flags: review?(peterv)
Also, you should document a bit more when somebody might want to use TenuredHeap.  Terrence said in IRC that things in the nursery can't use finalizers, which is presumably why you need this here.
Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

This is pretty much righteous. r=me, but I'd like to get a second reviewer for the mWrapper/mFlatJSObject changes. Those are extremely complicated. They appear right to me, but that code is complex enough that someone who really knows it should take a look.

::: content/base/src/nsContentUtils.cpp
@@ +1649,5 @@
>    if (!cx) {
>      return;
>    }
>    if (JSObject* global = js::GetDefaultGlobalForContext(cx)) {
> +    JS::AssertGCThingMustBeTenured(global);

Could we move this assertion into GetDefaultGlobalForContext?

::: dom/base/nsGlobalWindow.h
@@ +1224,5 @@
>    nsRefPtr<nsHistory>           mHistory;
>  
>    // These member variables are used on both inner and the outer windows.
>    nsCOMPtr<nsIPrincipal> mDocumentPrincipal;
> +  JS::TenuredHeap<JSObject*> mJSObject;

A comment explaining how we know that this is always tenured would be helpful.

::: js/public/RootingAPI.h
@@ +278,5 @@
> + * Additonally, we allow some flags to be stored in the lower bits of the
> + * pointer.
> + */
> +template <typename T>
> +class TenuredHeap : public js::HeapBase<T>

Olli and Andrew found this class quite confusing. They are okay with it with additional explanation, but they would like the comments on Heap and TenuredHeap and heap extended significantly.

Here is my take at a clearer comment on these classes. Olli, Andrew, Jon, what are your thoughts?

Heap
***********
The Heap<T> class is a C/C++ heap stored reference to a JS GC thing. GC references stored on the C/C++ stack must use Rooted/Handle/MutableHandle instead. Heap<T> wraps the complex mechanisms required to ensure GC safety for the contained reference into a C++ class that behaves similarly to a normal pointer.

Requirements for type T:
  - Must be one of: Value, jsid, JSObject*, JSString*
***********

TenuredHeap
***********
The TenuredHeap class is similar to the Heap class above in that it encapsulates the GC concerns of an on-heap reference to a JS object. However, it has two important differences:
  1) Pointers which are statically known to only reference "tenured" objects can avoid the extra overhead of SpiderMonkey's write barriers.
  2) Objects in the "tenured" heap have stronger alignment restrictions than those in the "nursery", so flag bits should only be stored on pointers known to be tenured. TenuredHeap wraps a normal tagged pointer with a nice API for accessing the flag bits and adds various assertions to ensure that it is not mis-used. 

GC things are said to be "tenured" when they are located in the long-lived heap: e.g. they have gained tenure as an object by surviving past at least one GC. For performance, SpiderMonkey allocates some things which are known to normally be long lived directly into the tenured generation; for example, global objects. Additionally, SpiderMonkey does not visit individual objects when deleting non-tenured objects, so object with finalizers are also always tenured; for instance, this includes most DOM objects.

The considerations to keep in mind when using a TenuredHeap<T> vs a normal Heap<T> are:
  - It is invalid for a TenuredHeap<T> to refer to a non-tenured thing.
  - It is invalid to store flag bits on a Heap<T>.
  - It is valid for Heap<T> to refer to a tenured thing.
***********

::: js/xpconnect/src/xpcprivate.h
@@ +1411,5 @@
>                          js::PointerHasher<JSObject *, 3>,
>                          js::SystemAllocPolicy> DOMExpandoSet;
>  
>      bool RegisterDOMExpandoObject(JSObject *expando) {
> +        JS::AssertGCThingMustBeTenured(expando);

A comment here explaining why all expandos are tenured would probably be helpful.

@@ +2475,1 @@
>          {

{ at end of previous line.

@@ +2578,5 @@
> +        WRAPPER_NEEDS_SOW = JS_BIT(0),
> +        WRAPPER_NEEDS_COW = JS_BIT(1),
> +        WRAPPER_DESTROYED = JS_BIT(2),
> +
> +        // Flags bits for mJSObject:

s/mJSObject/mFlatJSObject/

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +517,5 @@
>        continue;
>      }
>  
>      if (JSObject* global = js::GetDefaultGlobalForContext(acx)) {
> +      JS::AssertGCThingMustBeTenured(global);

And remove this with the assertion moved into the getter.
Attachment #772698 - Flags: review?(terrence)
Attachment #772698 - Flags: review?
Attachment #772698 - Flags: review+
Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

Ah, I see Andrew already added a second review. Thanks!
Attachment #772698 - Flags: review?
That seems a little wordy.  For instance, that Tenured can't hold non-Tenured things seems self-evident. ;)
(In reply to Andrew McCreight [:mccr8] from comment #18)

> I guess JS::Heap always initializes to zero?

Yes, Heap<T> initializes to NULL when T is a pointer and to JS::UndefinedValue() when it is JS::Value.
Comment on attachment 772701 [details] [diff] [review]
7 - barriers in workers

>+  class StateDataAutoRooter : private JS::CustomAutoRooter
>+  {
>+  public:
>+    explicit StateDataAutoRooter(JSContext *cx, XMLHttpRequest::StateData *aData
JSContext* aCx, XMLHttpRequest::StateData* aData

>+                                 MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>+    : CustomAutoRooter(cx), mStateData(aData), mSkip(cx, mStateData)
>+    {
>+      MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>+    }
>+
>+  private:
>+    virtual void trace(JSTracer *trc)
JSTracer* aTrc

>+    XMLHttpRequest::StateData *mStateData;
XMLHttpRequest::StateData* mStateData;
Attachment #772701 - Flags: review?(bugs) → review+
Attachment #771429 - Flags: review?(sphink) → review?(terrence)
Comment on attachment 771429 [details] [diff] [review]
4 - barrier ctypes

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

r=me
Attachment #771429 - Flags: review?(terrence) → review+
Other than what mccr8 said, I think the comments are good.
If we make changes to Heap + Handle handling, need to add some comment about it, hopefully
some example even.
Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

::: dom/base/nsGlobalWindow.cpp
@@ +3148,5 @@
>  {
>    MOZ_ASSERT(IsOuterWindow());
>    if (aObject == mJSObject) {
> +    mJSObject = nullptr;
> +    mJSObject.setFlags(OUTER_WINDOW_PROXY_FINALIZED);

I don't understand how this gives us the same functionality as before, since we never seem to assert OUTER_WINDOW_PROXY_FINALIZED? Accessing mJSObject after PoisonOuterWindowProxy was called would crash, accessing it after OuterWindowProxyFinalized is called won't.
(In reply to Peter Van der Beken [:peterv] from comment #31)

It looks like I missed the second part of the comment in nsOuterWindowProxy::finalize(), and only dealt with preventing EnsureScriptEnvironment() from doing anything when called after this.  

    // Ideally we would use OnFinalize here, but it's possible that
    // EnsureScriptEnvironment will later be called on the window, and we don't
    // want to create a new script object in that case. Therefore, we need to
    // write a non-null value that will reliably crash when dereferenced.

So if I assert that the OUTER_WINDOW_PROXY_FINALIZED is not set in methods that access mJSObject, would that be ok?
My main worry is that people will assume that null-checking is enough and not add a flag check. If they try to use it without null-checking then obviously we'll crash, so I'm not sure that adding OUTER_WINDOW_PROXY_FINALIZED checks everywhere is necessary. But not sure either what a good solution is, we could add a flag check in TenuredHeap, have a template argument to pass in the flag value to check for and add checks in the operators? Not sure it's worth it :-/.
In response to peterv's comment, I've changed this to add a method to TenuredHeap<T> to set the contents to a value that will cause a crash if it's dereferenced, so this now works basically the same as before.

Since it looks like he's away for the next few weeks, I'm asking bz to review.

Terrence has already reviewed the JS changes, see above.
Attachment #772698 - Attachment is obsolete: true
Attachment #772698 - Flags: review?(peterv)
Attachment #778515 - Flags: review?(bzbarsky)
Attachment #778515 - Attachment description: tenured-heap-class → 1 - Add TenuredHeap<T> class
Comment on attachment 778515 [details] [diff] [review]
1 - Add TenuredHeap<T> class

Shouldn't XPCWrappedNative::Destroy use the crash-on-touch thing too?

>+        {if (mFlatJSObject)
>              xpc_UnmarkGrayObject(mFlatJSObject);

xpc_UnmarkGrayObject is null-safe, so no need for the null check.

r=me with those fixed
Attachment #778515 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.