Closed Bug 975456 Opened 9 years ago Closed 9 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: nmatsakis, Assigned: nmatsakis)




(1 file, 2 obsolete files)

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.
Attached patch Bug975456.diff (obsolete) — Splinter Review
Attachment #8379817 - Flags: review?(sphink)
Attachment #8379817 - Flags: review?(sphink) → review+
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.
Attached patch Bug975456.diff (obsolete) — Splinter Review
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.
Attachment #8379906 - Flags: review?(sphink)
Attachment #8379817 - Attachment is obsolete: true
Attachment #8379906 - Flags: review?(sphink) → review+
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.
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).
Attached patch Bug975456.diffSplinter Review
Take 3. See previous comment.
Attachment #8379906 - Attachment is obsolete: true
Attachment #8380117 - Flags: review?(sphink)
Try run: (includes bug 972581 and bug 972569)

I tried this locally and it seemed to be passing -- hopefully the TBPL run will show the same. :)
Summary: Preserve invariant that views on a neutered buffer have a NULL data pointer → Establish invariant that views on a neutered buffer have a NULL data pointer
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.
Blocks: 972581
Comment on attachment 8380117 [details] [diff] [review]

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;

but maybe that's more cryptic. Whichever reads better to you.
Attachment #8380117 - Flags: review?(sphink) → review+
Assignee: nobody → nmatsakis
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Gary Kwong -- can you add me to cc list on bug 976697? I can't see what it is.
You need to log in before you can comment on or make changes to this bug.