Closed Bug 1344686 Opened 8 years ago Closed 8 years ago

Gray marking checks fail for Windows 7 devtools tests

Categories

(Core :: JavaScript: GC, defect)

Product:
Core
Component:
JavaScript: GC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr45 - wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(2 files)

bug1344686-fix-reg-exp-shared-barrier
(deleted), patch
sfink
: review+
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
bug1344686-backport
(deleted), patch
gchang
: approval-mozilla-beta+
jcristau
: approval-mozilla-esr45-
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review
One test failed three out of four times when the patches in bug 1335751 were pushed to inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3c4a6d36149c38695e0cdccd9e7bbe261d1e17c8&selectedJob=81712426 02:54:52 INFO - Found black to gray edge 1ADA15D8 02:54:52 INFO - from Object 0DB20280 RegExpShared code edge 02:54:52 INFO - from Script 0DB3A0B0 objects edge 02:54:52 INFO - from Object 10EE2DC0 script edge 02:54:52 INFO - from Object 10ED6900 object slot edge 02:54:52 INFO - from root nsXPCWrappedJS::mJSObj 02:54:52 INFO - Assertion failure: js::CheckGrayMarkingState(mJSContext), at c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/xpcom/base/CycleCollectedJSContext.cpp:1322
The RegExpShared's custom read barrier needs to also unmark gray like TenuredCell::readBarrier does. If we made RegExpShared a GC thing then this would happen automatically. This fixes the devtools test assertions on Windows.
Assignee: nobody → jcoppeard
Attachment #8844429 - Flags: review?(sphink)
Comment on attachment 8844429 [details] [diff] [review] bug1344686-fix-reg-exp-shared-barrier Review of attachment 8844429 [details] [diff] [review]: ----------------------------------------------------------------- \o/ I'm happy to see this fixed.
Attachment #8844429 - Flags: review?(sphink) → review+
I believe this issue goes back to at least 32 IIUC?
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/6351f15cf90b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: javascript-core-security → core-security-release
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3) Yes, this goes back to bug 1010441 which was in FF 32.
Blocks: 1010441
Flags: needinfo?(jcoppeard)
Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
Flags: needinfo?(jcoppeard)
Comment on attachment 8844429 [details] [diff] [review] bug1344686-fix-reg-exp-shared-barrier Approval Request Comment [Feature/Bug causing the regression]: Bug 1010441. [User impact if declined]: Possible crash / security vulnerability. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It's a simple change to add an expose call in the read barrier for RegExpShared. [String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8844429 - Flags: approval-mozilla-aurora?
Attached patch bug1344686-backport (deleted) — — Splinter Review
Approval Request Comment [Feature/Bug causing the regression]: Bug 1010441. [User impact if declined]: Possible crash / security vulnerability. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It's a simple change to add an expose call in the read barrier for RegExpShared. [String changes made/needed]: None.
Attachment #8849525 - Flags: approval-mozilla-beta?
Track 53+/54+/55+ as security issue.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox55: ? → +
Comment on attachment 8844429 [details] [diff] [review] bug1344686-fix-reg-exp-shared-barrier Fix a security issue. Aurora54+ & Beta53+.
Attachment #8844429 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8849525 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8849525 [details] [diff] [review] bug1344686-backport Not entirely sure this is worth backporting to ESR45 given where it is in its life cycle, but ESR52 seems prudent at least.
Attachment #8849525 - Flags: approval-mozilla-esr52?
Attachment #8849525 - Flags: approval-mozilla-esr45?
Setting qe-verify- based on Jon's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8849525 [details] [diff] [review] bug1344686-backport sec-moderate gc fix, let's take this for esr52 but not 45
Attachment #8849525 - Flags: approval-mozilla-esr52?
Attachment #8849525 - Flags: approval-mozilla-esr52+
Attachment #8849525 - Flags: approval-mozilla-esr45?
Attachment #8849525 - Flags: approval-mozilla-esr45-
Whiteboard: [adv-main53+][adv-esr52.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: