Closed
Bug 1401804
Opened 7 years ago
Closed 7 years ago
Intermittent application crashed [@ js::ProxyObject::setPrivate(JS::Value const&)] (Assertion failure: !JS::GCThingIsMarkedGray(JS::GCCellPtr(priv)), at js/src/vm/ProxyObject.cpp:131)
Categories
(Core :: JavaScript: GC, defect, P5)
Core
JavaScript: GC
Tracking
()
People
(Reporter: intermittent-bug-filer, Assigned: jonco)
References
Details
(5 keywords, Whiteboard: [adv-main57+][adv-esr52.5+][post-critsmash-triage])
Crash Data
Attachments
(3 files)
(deleted),
patch
|
sfink
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
lizzard
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
Updated•7 years ago
|
status-firefox57:
--- → affected
Keywords: assertion
Updated•7 years ago
|
Summary: Intermittent test_bug773962.xul | application crashed [@ js::ProxyObject::setPrivate(JS::Value const&)] (Assertion failure: !JS::GCThingIsMarkedGray(JS::GCCellPtr(priv)), at js/src/vm/ProxyObject.cpp:131) → Intermittent application crashed [@ js::ProxyObject::setPrivate(JS::Value const&)] (Assertion failure: !JS::GCThingIsMarkedGray(JS::GCCellPtr(priv)), at js/src/vm/ProxyObject.cpp:131)
Comment 1•7 years ago
|
||
This is new output from bug 1399866.
Blocks: 1399866
Flags: needinfo?(jcoppeard)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 2•7 years ago
|
||
I think this is most likely to be because the IsMarkedBlack() function used in these assertions currently returns true if the argument object is marked black or gray.
This assert is happening while we're remapping wrappers and this is telling us that the target is marked gray. Probably the wrapper is gray too.
Attachment #8910730 -
Flags: review?(sphink)
Updated•7 years ago
|
Attachment #8910730 -
Flags: review?(sphink) → review+
Comment 3•7 years ago
|
||
Bughunter sees this on Beta/57, Nightly/58 on Windows as well.
https://www.ithome.com/html/digi/326773.htm
Comment hidden (Intermittent Failures Robot) |
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56083ad02b19
Fix IsMarkedBlack check used in gray marking asserts r=sfink
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment hidden (Intermittent Failures Robot) |
Comment 13•7 years ago
|
||
Please request Beta approval on this.
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(jcoppeard)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8910730 [details] [diff] [review]
bug1401804-fix-marked-black
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1399866.
[User impact if declined]: None, requested because this is causing assertion failures in testing.
[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?]: This is simple change to relax an assertion.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8910730 -
Flags: approval-mozilla-beta?
Comment 16•7 years ago
|
||
Comment on attachment 8910730 [details] [diff] [review]
bug1401804-fix-marked-black
Fix an intermittent, taking it.
should be in 57b3
Attachment #8910730 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•7 years ago
|
||
uplift |
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
This crashes are still happening so this needs further investigation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•7 years ago
|
||
These crashes are happening when we are recomputing wrappers and we end up allocating a new wrapper and writing a gray target to it's private slot (creating a black->gray edge).
This patch adds an expose call for the target of a new wrapper to avoid this situation.
Attachment #8912725 -
Flags: review?(sphink)
Comment 22•7 years ago
|
||
Comment on attachment 8912725 [details] [diff] [review]
bug1401804-expose-wrappee
Review of attachment 8912725 [details] [diff] [review]:
-----------------------------------------------------------------
Nice catch!
Attachment #8912725 -
Flags: review?(sphink) → review+
Comment 23•7 years ago
|
||
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fdac23fbc5
Expose wrappee if we create a new wrapper r=sfink
Comment 24•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 25•7 years ago
|
||
Please request Beta approval on the second patch too :)
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 26•7 years ago
|
||
I should have marked this s-s when I uploaded the second patch in comment 21. This is an actual problem found by the new assertions in bug 1399866.
Group: javascript-core-security
Updated•7 years ago
|
Keywords: csectype-uaf,
sec-high
Updated•7 years ago
|
tracking-firefox57:
--- → ?
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8912725 [details] [diff] [review]
bug1401804-expose-wrappee
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1399866 highlighted the problem but it has been around for longer, at least back to 52.
[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?]: The patch adds an ExposeObjectToActiveJS call which is always safe (although might lead to higher memory use in pathological situations).
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8912725 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Updated•7 years ago
|
Attachment #8912725 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 28•7 years ago
|
||
Tracking as it is a sec-high
Comment 29•7 years ago
|
||
uplift |
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Comment 30•7 years ago
|
||
I believe we need ESR52 patches and an uplift request here, Jon?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 31•7 years ago
|
||
Rebased and combined patch for ESR52.
Flags: needinfo?(jcoppeard)
Attachment #8918256 -
Flags: review+
Assignee | ||
Comment 32•7 years ago
|
||
Comment on attachment 8918256 [details] [diff] [review]
bug1401804-esr52
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None
Attachment #8918256 -
Flags: approval-mozilla-esr52?
Comment on attachment 8918256 [details] [diff] [review]
bug1401804-esr52
Fix for sec-high issue, let's take this for 52.5.0esr.
Attachment #8918256 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Assignee | ||
Comment 34•7 years ago
|
||
uplift |
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [adv-main57+][adv-esr52.5+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•