Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725813002:
(CANCELED) Forbid GC during copyToVector() (Closed)
DescriptionForbid GC during copyToVector()
copyToVector() forbits GC while it resizes its incoming vecotr in the
previous patch[1].
This patch extends it further, forbids GC during the whole
copyToVector() operation. This can prevent GC collecting weak references
during iterations and de-referencing.
This is a speculative fix.
[1] http://crrev.com/1652953002
BUG=581698, 689949, 696231
Patch Set 1 #
Messages
Total messages: 19 (6 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== copyToVector BUG= ========== to ========== Forbid GC during copyToVector() copyToVector() forbits GC while it resizes its incoming vecotr in the previous patch[1]. This patch extends it further, forbids GC during the whole copyToVector() operation. This can prevent GC collecting weak references during iterations and de-referencing. This is a speculative fix. [1] http://crrev.com/1652953002 BUG=581698, 689949, 696231 ==========
kojii@chromium.org changed reviewers: + haraken@chromium.org, sigbjornf@opera.com
PTAL. This is #1 of https://crbug.com/696231#c13.
LGTM
The backing buffers are stack reachable during iteration, for any conservative GC, so I don't understand what this will fix.
I'm not an expert on memory, more on fonts staff. From the crash reports, after copyToVector, the vector contains nullptr. So are you saying begin(), end(), "++", and "*" never kicks GC and thus this change doesn't do anything? What are other possibility of the vector containing nullptr? From my perspective, if it's the designed behavior, we can add nullptr check in the loop, in FontCache and CSSFontSelector, but I saw you removed the nullptr check, so I suspect it's a temporary workaround we should do again, correct?
On 2017/03/01 06:28:30, kojii wrote: > I'm not an expert on memory, more on fonts staff. From the crash reports, after > copyToVector, the vector contains nullptr. > > So are you saying begin(), end(), "++", and "*" never kicks GC and thus this > change doesn't do anything? > Those shouldn't trigger heap allocations, but even if they did the backing store holding the collection will be detected when scanning the stack conservatively, and the references from it should be "strongified" if they're weak. This is an iteration like any other, so should we trust no iterations to behave correctly any longer in the face of GCs? > What are other possibility of the vector containing nullptr? > I haven't looked at this crasher yet, but that is the key thing to determine :) > From my perspective, if it's the designed behavior, we can add nullptr check in > the loop, in FontCache and CSSFontSelector, but I saw you removed the nullptr > check, so I suspect it's a temporary workaround we should do again, correct? That was added on the path to understanding the underlying problem at the time. I'm fine with trying this, not standing in the way, just asking questions.
On 2017/03/01 at 06:39:44, sigbjornf wrote: > On 2017/03/01 06:28:30, kojii wrote: > > I'm not an expert on memory, more on fonts staff. From the crash reports, after > > copyToVector, the vector contains nullptr. > > > > So are you saying begin(), end(), "++", and "*" never kicks GC and thus this > > change doesn't do anything? > > > > Those shouldn't trigger heap allocations, but even if they did the backing store holding the collection will be detected when scanning the stack conservatively, and the references from it should be "strongified" if they're weak. > > This is an iteration like any other, so should we trust no iterations to behave correctly any longer in the face of GCs? I agree on what you said, though my knowledge on Oilpan is very limited. > > What are other possibility of the vector containing nullptr? > > > > I haven't looked at this crasher yet, but that is the key thing to determine :) > > > From my perspective, if it's the designed behavior, we can add nullptr check in > > the loop, in FontCache and CSSFontSelector, but I saw you removed the nullptr > > check, so I suspect it's a temporary workaround we should do again, correct? > > That was added on the path to understanding the underlying problem at the time. I'm fine with trying this, not standing in the way, just asking questions. I see. I'm a font guy, involved due to the signature and wanted to help RB if there were anything I can do in APAC timezone. I can't really explain how this change could fix, but I also can't say what's the problem is. If you want to look at in your convenient time, I'm fine not to land this and leave the 3 issues to you. Does that work better for you? I also prepared a CL to skip nullptr, this is more likely to fix (or suppress?) the crash, but I guess this isn't a right fix and you don't want to land, no? https://codereview.chromium.org/2723033002
On 2017/03/01 06:49:49, kojii wrote: > On 2017/03/01 at 06:39:44, sigbjornf wrote: > > On 2017/03/01 06:28:30, kojii wrote: > > > I'm not an expert on memory, more on fonts staff. From the crash reports, > after > > > copyToVector, the vector contains nullptr. > > > > > > So are you saying begin(), end(), "++", and "*" never kicks GC and thus this > > > change doesn't do anything? > > > > > > > Those shouldn't trigger heap allocations, but even if they did the backing > store holding the collection will be detected when scanning the stack > conservatively, and the references from it should be "strongified" if they're > weak. > > > > This is an iteration like any other, so should we trust no iterations to > behave correctly any longer in the face of GCs? > > I agree on what you said, though my knowledge on Oilpan is very limited. > > > > What are other possibility of the vector containing nullptr? > > > > > > > I haven't looked at this crasher yet, but that is the key thing to determine > :) > > > > > From my perspective, if it's the designed behavior, we can add nullptr check > in > > > the loop, in FontCache and CSSFontSelector, but I saw you removed the > nullptr > > > check, so I suspect it's a temporary workaround we should do again, correct? > > > > That was added on the path to understanding the underlying problem at the > time. I'm fine with trying this, not standing in the way, just asking questions. > > I see. I'm a font guy, involved due to the signature and wanted to help RB if > there were anything I can do in APAC timezone. > > I can't really explain how this change could fix, but I also can't say what's > the problem is. If you want to look at in your convenient time, I'm fine not to > land this and leave the 3 issues to you. Does that work better for you? > > I also prepared a CL to skip nullptr, this is more likely to fix (or suppress?) > the crash, but I guess this isn't a right fix and you don't want to land, no? > https://codereview.chromium.org/2723033002 I wouldn't mind doing that one as a first step. If the crash stops reproducing, then we would know that no addClient() have added empties _and_ that a GC hadn't copied in a cleared weak reference into the vector during copyToVector().
On 2017/03/01 06:58:53, sof wrote: > On 2017/03/01 06:49:49, kojii wrote: > > On 2017/03/01 at 06:39:44, sigbjornf wrote: > > > On 2017/03/01 06:28:30, kojii wrote: > > > > I'm not an expert on memory, more on fonts staff. From the crash reports, > > after > > > > copyToVector, the vector contains nullptr. > > > > > > > > So are you saying begin(), end(), "++", and "*" never kicks GC and thus > this > > > > change doesn't do anything? > > > > > > > > > > Those shouldn't trigger heap allocations, but even if they did the backing > > store holding the collection will be detected when scanning the stack > > conservatively, and the references from it should be "strongified" if they're > > weak. > > > > > > This is an iteration like any other, so should we trust no iterations to > > behave correctly any longer in the face of GCs? > > > > I agree on what you said, though my knowledge on Oilpan is very limited. > > > > > > What are other possibility of the vector containing nullptr? > > > > > > > > > > I haven't looked at this crasher yet, but that is the key thing to determine > > :) > > > > > > > From my perspective, if it's the designed behavior, we can add nullptr > check > > in > > > > the loop, in FontCache and CSSFontSelector, but I saw you removed the > > nullptr > > > > check, so I suspect it's a temporary workaround we should do again, > correct? > > > > > > That was added on the path to understanding the underlying problem at the > > time. I'm fine with trying this, not standing in the way, just asking > questions. > > > > I see. I'm a font guy, involved due to the signature and wanted to help RB if > > there were anything I can do in APAC timezone. > > > > I can't really explain how this change could fix, but I also can't say what's > > the problem is. If you want to look at in your convenient time, I'm fine not > to > > land this and leave the 3 issues to you. Does that work better for you? > > > > I also prepared a CL to skip nullptr, this is more likely to fix (or > suppress?) > > the crash, but I guess this isn't a right fix and you don't want to land, no? > > https://codereview.chromium.org/2723033002 > > I wouldn't mind doing that one as a first step. If the crash stops reproducing, > then we would know that no addClient() have added empties _and_ that a GC hadn't > copied in a cleared weak reference into the vector during copyToVector(). The logic in that sentence is a bit off, but you get my point, i'm sure -- we would know that it was the copyToVector() iteration that caused the failure.
On 2017/03/01 at 07:03:31, sigbjornf wrote: > > > > > > I also prepared a CL to skip nullptr, this is more likely to fix (or > > suppress?) > > > the crash, but I guess this isn't a right fix and you don't want to land, no? > > > https://codereview.chromium.org/2723033002 > > > > I wouldn't mind doing that one as a first step. If the crash stops reproducing, > > then we would know that no addClient() have added empties _and_ that a GC hadn't > > copied in a cleared weak reference into the vector during copyToVector(). > > The logic in that sentence is a bit off, but you get my point, i'm sure -- we would know that it was the copyToVector() iteration that caused the failure. Allow me to double-confirm, my English isn't good, sorry. So you think "check and skip nullptr" is better than this CL, it'll tell us what is actually happening?
On 2017/03/01 07:11:17, kojii wrote: > On 2017/03/01 at 07:03:31, sigbjornf wrote: > > > > > > > > I also prepared a CL to skip nullptr, this is more likely to fix (or > > > suppress?) > > > > the crash, but I guess this isn't a right fix and you don't want to land, > no? > > > > https://codereview.chromium.org/2723033002 > > > > > > I wouldn't mind doing that one as a first step. If the crash stops > reproducing, > > > then we would know that no addClient() have added empties _and_ that a GC > hadn't > > > copied in a cleared weak reference into the vector during copyToVector(). > > > > The logic in that sentence is a bit off, but you get my point, i'm sure -- we > would know that it was the copyToVector() iteration that caused the failure. > > Allow me to double-confirm, my English isn't good, sorry. > > So you think "check and skip nullptr" is better than this CL, it'll tell us what > is actually happening? Yes, a better first step.
Thank you so much for your prompt support. I sent that one to your review, and put "(Pending)" to this title.
On 2017/03/01 07:33:18, kojii wrote: > Thank you so much for your prompt support. > > I sent that one to your review, and put "(Pending)" to this title. Thanks for being willing to explore that first, I hope there's time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Canceling this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
