975456 - Establish invariant that views on a neutered buffer have a NULL data pointer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Currently, when a buffer is neutered, we set the data pointer on all views to NULL, but then we reset the pointer during tracing. This foils attempts to check for neutered typed objects by comparing the data pointer to NULL (note that typed arrays always check for neutered views using the length). This patch just checks in the trace method whether the buffer is neutered and, if so, leaves the data pointer as NULL.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug975456.diff

Comment 2

[Niko Matsakis [:nmatsakis]]

Note: the problem crops up with the existing neutering tests when run with --enable-gczeal and --enable-gcgenerational, when combined with the patches for bug 972581 (which ought to be orthogonal, but I wasn't seeing this particular problem before testing those patches), so I didn't add a test.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug975456.diff

Testing reveals I forgot one case. I didn't know if this func was at all perf sensitive, so I opted to pull the if out of the loop, at the cost of some (relatively slight) code duplication. Easily switched otherwise if you think better.

Comment 4

[Brian Hackett [Laid off!]]

I was running into test failures in TypedObject/neutertypedobj.js and TypedObject/neutertypedobjsizedarray.js as part of bug 964057, so disabled both these tests.  When this bug lands I think the tests will be fixed and they can be reenabled.

Comment 5

[Niko Matsakis [:nmatsakis]]

Gah. Sorry. It turns out that the previous 2 patches weren't QUITE right. And I guess the title of this bug wasn't right: this patch isn't preserving an invariant so much as *establishing* one, since it seems that I was wrong to think that the data pointer would be NULL if the view had been neutered. One option is just to drop this invariant. But it seems like a nice invariant to have, so I'm a bit reluctant to do that. 

The problem with the previous patch is that, under some code paths, the views are neutered before changeContents() are called *and* before the array buffer itself is neutered. 

There are two options I see to fix this:

1. In changeContents(), check for a NULL data pointer and don't update the new view data pointer in that case. This is what I did so far and it works (all tests pass, including the crashtests, which I had not tried before posting my previous attempts). I will upload a revised patch shortly.

2. Move the neutering of views so that it takes place after the neutering of the buffer, not before. I didn't try this because of the comment on `neuter()` which states that, for asm.js buffers, this should take place after neutering views (though I don't quite grok what dependency there is).

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Bug975456.diff

Take 3. See previous comment.

Comment 7

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0308eed3b859 (includes bug 972581 and bug 972569)

I tried this locally and it seemed to be passing -- hopefully the TBPL run will show the same. :)

Comment 8

[Niko Matsakis [:nmatsakis]]

Try run looks pretty clean. There is one unexplained intermittent crash in Android 4.0 OPT R6 for which TBPL, at least, didn't uncover a known bug report, but I don't see any particular connection to this change. I haven't dug deeply yet to see if I can find other instances of this failure in recent builds.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8380117 [details] [diff] [review]
Bug975456.diff

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +402,5 @@
> +        // that the view has been neutered. In that case, the buffer
> +        // is "en route" to being neutered but the isNeuteredBuffer()
> +        // flag may not yet be set.
> +        uintptr_t oldViewPtr = uintptr_t(view->getPrivate());
> +        if (oldViewPtr) {

Ugh, sorry this is such a mess.

Can you rename these to oldViewDataPtr/newViewDataPtr? Or maybe just viewDataPtr:

  viewDataPtr = getPrivate();
  if (viewDataPtr) {
    viewDataPtr += newDataPointer - oldDataPointer;
    view->setPrivate(viewDataPtr);
  }

but maybe that's more cryptic. Whichever reads better to you.

Comment 10

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03355461606c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03355461606c

Comment 12

[Niko Matsakis [:nmatsakis]]

Gary Kwong -- can you add me to cc list on bug 976697? I can't see what it is.