Add neuter() variants to vary data pointer during tests

RESOLVED FIXED in Firefox 30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla31
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, firefox-esr2430+ fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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().
(Assignee)

Comment 1

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Comment 5

5 years ago
jsfunfuzz updated (709c72a0ec05)
Depends on: 1002864
Posted 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)
(Assignee)

Updated

5 years ago
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+
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+
(Assignee)

Updated

5 years ago
Attachment #8428134 - Flags: checkin+
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed
status-firefox30: --- → fixed
status-firefox31: --- → fixed
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)
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)
status-b2g-v1.3T: --- → fixed
(Assignee)

Updated

5 years ago
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+
tracking-firefox-esr24: --- → 30+
You need to log in before you can comment on or make changes to this bug.