Closed Bug 999755 Opened 10 years ago Closed 10 years ago

Add neuter() variants to vary data pointer during tests

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 30+ fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(3 files, 2 obsolete files)

When structured clone neuters an ArrayBuffer, it may leave the data pointer unmodified (for asm.js buffers) or modified (currently, for everything else). We should expose both via the test function neuter().
Attachment #8410612 - Flags: review?(jwalden+bmo)
Comment on attachment 8410612 [details] [diff] [review]
Add neuter() variants to vary data pointer

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

::: js/src/builtin/TestingFunctions.cpp
@@ +1465,5 @@
>      }
>  
> +    NeuterDataDisposition changeData = ChangeData;
> +    if (args.length() >= 1) {
> +        JSString *str = JS::ToString(cx, args[1]);

Rooted for sanity?

I'd prefer if this argument were mandatory, and everyone had to pass it.  Aside from the churn to tests, clearly EIBTI and all that here.  How strongly do you object to doing this?
Attachment #8410612 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/e9cb7cf27ce7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
jsfunfuzz updated (709c72a0ec05)
Depends on: 1002864
Attached patch Beta-targeted patch (obsolete) — Splinter Review
It would be helpful, on occasion, to be able to run tests targeted at trunk against beta, with all the data-disposition bits from this addition to our testing capabilities.  This patch consists of the patch from this bug, plus the patch from bug 1002864, with the necessary adjustments for the beta branch (including not changing existing interface signatures), as far as I can tell.  It passes jit-tests, so I'm reasonably confident it's all in order.  But it's sufficiently different from what's already landed to need review, I think.
Attachment #8424378 - Flags: review?(sphink)
Attachment #8424378 - Attachment is obsolete: true
Attachment #8424378 - Flags: review?(sphink)
Attachment #8425624 - Flags: review?(sphink)
Attachment #8425624 - Flags: review?(sphink) → review+
Comment on attachment 8425624 [details] [diff] [review]
Beta-targeted: Don't change existing JSAPI signatures, for reals

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: lessened ability to reuse tests written for trunk against beta
Testing completed (on m-c, etc.): landed on m-c and aurora, although forms differ slightly
Risk to taking this patch (and alternatives if risky): low; bunch of tests to exercise the changes, concept proven on trunk/aurora
String or IDL/UUID changes made by this patch: N/A
Attachment #8425624 - Flags: approval-mozilla-beta?
This sounds like a nice-to-have but we're pretty far along in the Beta cycle so why not wait the 3 weeks until 31 is on Beta? Comment 6 suggests this is not oft-used practice and since this isn't a tracked issue, I'm inclined towards holding back on additional churn on the branch.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8425624 [details] [diff] [review]
Beta-targeted: Don't change existing JSAPI signatures, for reals

Given the low-risk and the help this provides to developers on current work, we will take this for uplift.
Attachment #8425624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch Final beta patchSplinter Review
One last little tweak in response to try-bustage on this, reviewed by sfink over IRC, and this is good to go.  I'd push now, but I don't have time to watch the tree.  Maybe tomorrow morning if no one beats me to it.
Attachment #8425624 - Attachment is obsolete: true
Attachment #8428134 - Flags: review+
Attachment #8428134 - Flags: checkin+
Comment on attachment 8428134 [details] [diff] [review]
Final beta patch

Same justification as for beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: harder to run various tests against b2g28/b2g26
Testing completed: local testing of various neutering patchwork, some tryservering of this patch in concert with other work (details available upon request)
Risk to taking this patch (and alternatives if risky): pretty low, backport is pretty simple
String or UUID changes made by this patch: N/A

I still should be the person landing this patchwork, for simplicity.
Attachment #8428134 - Flags: approval-mozilla-b2g28?
Attachment #8428134 - Flags: approval-mozilla-b2g26?
Flags: needinfo?(bbajaj)
Attachment #8428134 - Flags: approval-mozilla-b2g28?
Attachment #8428134 - Flags: approval-mozilla-b2g28+
Attachment #8428134 - Flags: approval-mozilla-b2g26?
Attachment #8428134 - Flags: approval-mozilla-b2g26+
Flags: needinfo?(bbajaj)
Attached patch esr24 backportSplinter Review
The neuter function doesn't even exist in esr24, so this is kind of totally newish.  It definitely has strong similarities in parts to the previously reviewed patches, tho, so it's reasonably simple in any event.
Attachment #8431288 - Flags: review?(sphink)
Attachment #8431288 - Flags: review?(sphink) → review+
Comment on attachment 8431288 [details] [diff] [review]
esr24 backport

Approval request comments from the aurora, beta, and b2g* requests apply.
Attachment #8431288 - Flags: approval-mozilla-esr24?
Comment on attachment 8431288 [details] [diff] [review]
esr24 backport

ESR24 approval granted in support of other fixes.
Attachment #8431288 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
You need to log in before you can comment on or make changes to this bug.