[Oilpan] Remove a GC_PLUGIN_IGNORE

Remove a GC_PLUGIN_IGNORE for an issue which has already been fixed.


BUG=516604, 496496
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201624

[peria, 2015-09-01 06:54:00]

peria@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[peria, 2015-09-01 06:54:00]

PTL

[sof, 2015-09-01 07:12:36]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-09-01 07:12:36]

lgtm

[peria, 2015-09-01 07:14:33]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-09-01 07:14:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320443005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320443005/1

[commit-bot: I haz the power, 2015-09-01 07:23:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-01 07:23:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[peria, 2015-09-01 07:26:41]

peria@chromium.org changed reviewers:
+ kbr@chromium.org

[peria, 2015-09-01 07:26:41]

PTL

[haraken, 2015-09-01 07:29:14]

> m_textureUnits will be cleared at the end of the dtor

This is not correct. It's not cleared at the end of the destructor.

I thought that the =nullptr was needed in order not to let the subsequent
detachAndRemoveAllObjects() touch m_textureUnits, but it seems that there is no
code path that touches m_textureUnits in detachAndRemoveAllObjects().

Hmm, I guess it is safe to remove the =nullptr but want to have another eye on
this.

[haraken, 2015-09-01 07:29:40]

On 2015/09/01 07:29:14, haraken wrote:
> > m_textureUnits will be cleared at the end of the dtor
> 
> This is not correct. It's not cleared at the end of the destructor.
> 
> I thought that the =nullptr was needed in order not to let the subsequent
> detachAndRemoveAllObjects() touch m_textureUnits, but it seems that there is
no
> code path that touches m_textureUnits in detachAndRemoveAllObjects().
> 
> Hmm, I guess it is safe to remove the =nullptr but want to have another eye on
> this.

OK, Sigbjorn looks happy. LGTM.

[sof, 2015-09-01 07:50:34]

On 2015/09/01 07:29:40, haraken wrote:
> On 2015/09/01 07:29:14, haraken wrote:
> > > m_textureUnits will be cleared at the end of the dtor
> > 
> > This is not correct. It's not cleared at the end of the destructor.
> > 
> > I thought that the =nullptr was needed in order not to let the subsequent
> > detachAndRemoveAllObjects() touch m_textureUnits, but it seems that there is
> no
> > code path that touches m_textureUnits in detachAndRemoveAllObjects().
> > 
> > Hmm, I guess it is safe to remove the =nullptr but want to have another eye
on
> > this.
> 
> OK, Sigbjorn looks happy. LGTM.

Looking more carefully, not sure I have well-founded reasons to be so, as we may
end up doing unintended extra work here.

The WebGLTexture shared objects will be detached & deleted by
WebGLRenderingContextBase (when it calls removeContext()), which will bring
about deleteTexture() calls.

So, either leave the nullptr-filling loop as it is or call
m_textureUnits.clear() instead (and have an early return for
!m_textureUnits.size() in deleteTexture()), seems preferable.

[Ken Russell (switch to Gerrit), 2015-09-02 00:04:28]

On 2015/09/01 07:50:34, sof wrote:
> On 2015/09/01 07:29:40, haraken wrote:
> > On 2015/09/01 07:29:14, haraken wrote:
> > > > m_textureUnits will be cleared at the end of the dtor
> > > 
> > > This is not correct. It's not cleared at the end of the destructor.
> > > 
> > > I thought that the =nullptr was needed in order not to let the subsequent
> > > detachAndRemoveAllObjects() touch m_textureUnits, but it seems that there
is
> > no
> > > code path that touches m_textureUnits in detachAndRemoveAllObjects().
> > > 
> > > Hmm, I guess it is safe to remove the =nullptr but want to have another
eye
> on
> > > this.
> > 
> > OK, Sigbjorn looks happy. LGTM.
> 
> Looking more carefully, not sure I have well-founded reasons to be so, as we
may
> end up doing unintended extra work here.
> 
> The WebGLTexture shared objects will be detached & deleted by
> WebGLRenderingContextBase (when it calls removeContext()), which will bring
> about deleteTexture() calls.
> 
> So, either leave the nullptr-filling loop as it is or call
> m_textureUnits.clear() instead (and have an early return for
> !m_textureUnits.size() in deleteTexture()), seems preferable.

Sorry, I didn't get to this review today. I defer to sof@, so lgtm as necessary
for presubmit checks, but I do plan to investigate soon whether these
m_contextObjects and m_sharedObjects can just be removed at this point. Will
file a bug about this probably tomorrow.

[peria, 2015-09-02 03:40:51]

On 2015/09/01 07:50:34, sof wrote:
> Looking more carefully, not sure I have well-founded reasons to be so, as we
may
> end up doing unintended extra work here.
> 
> The WebGLTexture shared objects will be detached & deleted by
> WebGLRenderingContextBase (when it calls removeContext()), which will bring
> about deleteTexture() calls.
> 
> So, either leave the nullptr-filling loop as it is or call
> m_textureUnits.clear() instead (and have an early return for
> !m_textureUnits.size() in deleteTexture()), seems preferable.

Thank you for the investigation.
I will follow your suggestions.


On 2015/09/02 00:04:28, Ken Russell wrote:
> Sorry, I didn't get to this review today. I defer to sof@, so lgtm as
necessary
> for presubmit checks, but I do plan to investigate soon whether these
> m_contextObjects and m_sharedObjects can just be removed at this point. Will
> file a bug about this probably tomorrow.

I'm glad if they can. :)

[peria, 2015-09-02 05:31:02]

I thought for a while, and decide not to change code for now.
(I chose the first one in sof@'s suggestions.)
This CL just removes a GC_PLUGIN_IGNORE for a fixed issue.

PTAL for PS2.

[haraken, 2015-09-02 05:31:55]

LGTM

[peria, 2015-09-02 05:33:56]

The CQ bit was checked by peria@chromium.org

[peria, 2015-09-02 05:33:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1320443005/#ps20001
(title: "Revert and remove GC_PLUGIN_IGNORE")

[commit-bot: I haz the power, 2015-09-02 05:34:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320443005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320443005/20001

[sof, 2015-09-02 05:38:14]

I would prefer a comment next to the loop, explaining its purpose & why it is
valid, as there's about half a dozen indirect subtleties involved here. :)

[peria, 2015-09-02 05:38:55]

The CQ bit was unchecked by peria@chromium.org

[peria, 2015-09-02 05:50:03]

On 2015/09/02 05:38:14, sof wrote:
> I would prefer a comment next to the loop, explaining its purpose & why it is
> valid, as there's about half a dozen indirect subtleties involved here. :)

Yeah, it should be written.

[sof, 2015-09-02 06:15:21]

On 2015/09/02 05:50:03, peria wrote:
> On 2015/09/02 05:38:14, sof wrote:
> > I would prefer a comment next to the loop, explaining its purpose & why it
is
> > valid, as there's about half a dozen indirect subtleties involved here. :)
> 
> Yeah, it should be written.

thanks, lgtm.

[peria, 2015-09-02 06:19:25]

The CQ bit was checked by peria@chromium.org

[peria, 2015-09-02 06:19:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1320443005/#ps40001
(title: "Add a comment")

[commit-bot: I haz the power, 2015-09-02 06:19:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320443005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320443005/40001

[commit-bot: I haz the power, 2015-09-02 07:30:48]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201624