1401804 - Intermittent application crashed [@ js::ProxyObject::setPrivate(JS::Value const&)] (Assertion failure: !JS::GCThingIsMarkedGray(JS::GCCellPtr(priv)), at js/src/vm/ProxyObject.cpp:131)

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=132347212&repo=try

https://queue.taskcluster.net/v1/task/Y7sxIm84SNa6o-DHB0JGgA/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is new output from bug 1399866.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1401804-fix-marked-black

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.

Comment 3

[Bob Clary [:bc] (inactive)]

Bughunter sees this on Beta/57, Nightly/58 on Windows as well.
https://www.ithome.com/html/digi/326773.htm

Comment 5

[Intermittent Failures Robot]

16 failures in 199 pushes (0.08 failures/push) were associated with this bug yesterday.    

Repository breakdown:
* mozilla-inbound: 5
* mozilla-central: 3
* mozilla-beta: 3
* autoland: 3
* try: 2

Platform breakdown:
* windows7-32: 7
* windows7-32-stylo-disabled: 4
* osx-10-10: 2
* windows10-64: 1
* macosx64-stylo-disabled: 1
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1401804&startday=2017-09-21&endday=2017-09-21&tree=all

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56083ad02b19
Fix IsMarkedBlack check used in gray marking asserts r=sfink

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/56083ad02b19

Comment 11

[Intermittent Failures Robot]

19 failures in 139 pushes (0.137 failures/push) were associated with this bug yesterday.    

Repository breakdown:
* mozilla-beta: 9
* autoland: 7
* mozilla-inbound: 3

Platform breakdown:
* windows7-32: 7
* windows7-32-stylo-disabled: 6
* linux64: 2
* osx-10-10: 1
* macosx64-stylo-disabled: 1
* linux64-stylo-disabled: 1
* linux32-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1401804&startday=2017-09-22&endday=2017-09-22&tree=all

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this.

Comment 14

[Intermittent Failures Robot]

40 failures in 943 pushes (0.042 failures/push) were associated with this bug in the last 7 days. 

This is the #42 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **  

Repository breakdown:
* mozilla-beta: 19
* mozilla-inbound: 9
* autoland: 8
* try: 2
* mozilla-central: 2

Platform breakdown:
* windows7-32: 16
* windows7-32-stylo-disabled: 9
* osx-10-10: 6
* linux64: 4
* macosx64-stylo-disabled: 2
* windows10-64: 1
* linux64-stylo-disabled: 1
* linux32-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1401804&startday=2017-09-18&endday=2017-09-24&tree=all

Comment 15

[Jon Coppeard (:jonco)]

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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910730 [details] [diff] [review]
bug1401804-fix-marked-black

Fix an intermittent, taking it.
should be in 57b3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Jon uplifted this.
https://hg.mozilla.org/releases/mozilla-beta/rev/acd879464984d82b5510009773b1aa48ce7381ff

Comment 18

[Jon Coppeard (:jonco)]

This crashes are still happening so this needs further investigation.

Comment 21

[Jon Coppeard (:jonco)]

Attachment: bug1401804-expose-wrappee

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.

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8912725 [details] [diff] [review]
bug1401804-expose-wrappee

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

Nice catch!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fdac23fbc5
Expose wrappee if we create a new wrapper r=sfink

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/60fdac23fbc5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on the second patch too :)

Comment 26

[Jon Coppeard (:jonco)]

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.

Comment 27

[Jon Coppeard (:jonco)]

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.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Tracking as it is a sec-high

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d0134315fc7d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

I believe we need ESR52 patches and an uplift request here, Jon?

Comment 31

[Jon Coppeard (:jonco)]

Attachment: bug1401804-esr52

Rebased and combined patch for ESR52.

Comment 32

[Jon Coppeard (:jonco)]

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

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8918256 [details] [diff] [review]
bug1401804-esr52

Fix for sec-high issue, let's take this for 52.5.0esr.

Comment 34

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2a87fb6b9c07cce17663da4b1f06e97c9ffff8e6