Closed Bug 1385512 Opened 7 years ago Closed 7 years ago

Intermittent bug1109328.js | bug1109328.js:7:1 Error: attempt to set maxBytes to the value less than the current gcBytes (1007616) (code 3, args "--baseline-eager")

Categories

(Core :: JavaScript Engine, defect, P5)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: sfink)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:logic])

Attachments

(1 file, 1 obsolete file)

Do not race with gcBytes
714 bytes, patch
jonco
: review+
Details | Diff | Splinter Review 
a recent increase in failures, but not enough to focus on the bug
seems like this picked up in the last week.

:naveed, I see you as the triage owner, could you help prioritize this and find someone to own this?
Flags: needinfo?(nihsanullah)
Whiteboard: [stockwell needswork]
Attached patch Do not race with gcBytes (obsolete) — — Splinter Review 
This is failing most of the time on the tier 3 Windows SM(cgc) jobs. I'm not sure why it's failing so frequently, but it seems inherently racy. The relevant code is doing

  gcparam("maxBytes", gcparam("gcBytes"));

As best I can figure, this must grab out the current number of remaining GC bytes, then some other thread comes along and decrements gcBytes, and now you're trying to set maxBytes to something smaller than your current usage.

I don't know exactly what was required for the original test, so I'm just doing a blind modification to something similar that won't race.
Attachment #8908684 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8908684 [details] [diff] [review]
Do not race with gcBytes

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

I agree with yoyur analysis.

::: js/src/jit-test/tests/debug/bug1109328.js
@@ +3,5 @@
>  } catch (e) {}
>  g = newGlobal()
>  g.parent = this
>  g.eval("Debugger(parent).onExceptionUnwind=(function(){})");
> +gcparam("maxBytes", gcparam("maxBytes") - 8);

But I think this changes the meaning of the test though?

I'd be totally fine with adding |gczeal(0)| so we don't trigger GCs that can race with us.
(In reply to Jon Coppeard (:jonco) from comment #10)
> Comment on attachment 8908684 [details] [diff] [review]
> Do not race with gcBytes
> 
> Review of attachment 8908684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with yoyur analysis.
> 
> ::: js/src/jit-test/tests/debug/bug1109328.js
> @@ +3,5 @@
> >  } catch (e) {}
> >  g = newGlobal()
> >  g.parent = this
> >  g.eval("Debugger(parent).onExceptionUnwind=(function(){})");
> > +gcparam("maxBytes", gcparam("maxBytes") - 8);
> 
> But I think this changes the meaning of the test though?
> 
> I'd be totally fine with adding |gczeal(0)| so we don't trigger GCs that can
> race with us.


It does change the meaning of the test, but (1) the bug it was testing is fixed now, and (2) all I can think of is that this test was using this maxBytes setting to trigger a GC. So avoiding GC triggers is going to change the test in the same way.

I'm not confident of my analysis, but how about this: I'll leave it as decrementing maxBytes, but I'll also insert an explicit GC call just in case that's all that was going on. Otherwise, I'm not sure how to do this in a non-racy way.
Flags: needinfo?(nihsanullah)
There's a good chance this wouldn't actually trigger the original bug, since it may cause a GC in at some awkward time during shutdown instead of during the script execution, but I'm not sure what to do about that other than removing the maxBytes < gcBytes check and making it internally clamp to gcBytes. But it seems like it probably catches more mistakes the way it is.
Attachment #8908735 - Flags: review?(jcoppeard)
Attachment #8908684 - Attachment is obsolete: true
Attachment #8908684 - Flags: review?(jcoppeard)
Comment on attachment 8908735 [details] [diff] [review]
Do not race with gcBytes

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

Sorry, I didn't explain that very well.

This test is only failing in the CGC test runs with zeal mode 10 enabled.  So I was suggesting something that didn't affect the operation of the test when there was no zeal mode active.

Having said that... any test that uses a mechanism like this to trigger a GC at just the right spot is bogus and fragile and is almost certainly not exercising the same code path as it did when it was added.  So it doesn't really matter either way.
Attachment #8908735 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #13)
> Comment on attachment 8908735 [details] [diff] [review]
> Do not race with gcBytes
> 
> Review of attachment 8908735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I didn't explain that very well.
> 
> This test is only failing in the CGC test runs with zeal mode 10 enabled. 
> So I was suggesting something that didn't affect the operation of the test
> when there was no zeal mode active.

Ah! Sorry, obvious in retrospect.

> Having said that... any test that uses a mechanism like this to trigger a GC
> at just the right spot is bogus and fragile and is almost certainly not
> exercising the same code path as it did when it was added.  So it doesn't
> really matter either way.

Yeah, I'd rather not avoid the observed race with gczeal, because it leaves a race in place that could come back if we start doing something else that touches gcBytes offthread.

Thanks!
https://hg.mozilla.org/mozilla-central/rev/c385f15231d2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Whiteboard: [stockwell unknown] → [stockwell fixed:logic]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: