Add an assertion to make sure NodeRenderingTraversal::parent is called after the distribution resul…

Add an assertion to make sure NodeRenderingTraversal::parent is not called with a dirty distribution result.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180332

[hayato, 2014-08-11 04:17:15]

As long as I can tell from the result of tryserver, it might be okay to enable
this assertion.

This assertion would be helpful to prevent someone from calling
NodeRendertingTraversal with invalid state.

[esprehn, 2014-08-11 06:12:16]

On 2014/08/11 04:17:15, hayato wrote:
> As long as I can tell from the result of tryserver, it might be okay to enable
> this assertion.
> 
> This assertion would be helpful to prevent someone from calling
> NodeRendertingTraversal with invalid state.

Looks like you're hitting asserts on the try server?

[hayato, 2014-08-11 09:43:14]

You are right. :)

There are a couple of layout tests which hit the assertion.
Let me have a look.

[hayato, 2014-08-13 07:58:01]

PTAL.

I've fixed a crash. I think sooner is better to enable this assert as long as it
doesn't break any tests.

[esprehn, 2014-08-13 09:10:51]

See also https://codereview.chromium.org/446593004 and the discussion there.

https://codereview.chromium.org/448893003/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/ElementResolveContext.cpp (right):

https://codereview.chromium.org/448893003/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/ElementResolveContext.cpp:46:
element.document().updateDistributionForNodeIfNeeded(&element);
This is not correct, you shouldn't be updating distribution in the middle of
recalc style. Please put this at the top of the callstack that calls into this
code.

[hayato, 2014-08-14 09:02:40]

On 2014/08/13 09:10:51, esprehn wrote:
> See also https://codereview.chromium.org/446593004 and the discussion there.
> 
>
https://codereview.chromium.org/448893003/diff/40001/Source/core/css/resolver...
> File Source/core/css/resolver/ElementResolveContext.cpp (right):
> 
>
https://codereview.chromium.org/448893003/diff/40001/Source/core/css/resolver...
> Source/core/css/resolver/ElementResolveContext.cpp:46:
> element.document().updateDistributionForNodeIfNeeded(&element);
> This is not correct, you shouldn't be updating distribution in the middle of
> recalc style. Please put this at the top of the callstack that calls into this
> code.

Thank you for the review. The next patch has been uploaded and waiting for a
result of trybots.

[hayato, 2014-08-15 05:36:57]

Looks no crashes.

[hayato, 2014-08-15 05:47:40]

PTAL

[hayato, 2014-08-15 05:55:58]

On 2014/08/15 05:47:40, hayato wrote:
> PTAL

I noticed there is a similar patch under the review,
https://codereview.chromium.org/446593004, and you already mentioned that in
comment #5.
This is just a a coincidence. :)

After I land this patch, I will land 
https://codereview.chromium.org/448593002/.

[esprehn, 2014-08-15 06:04:53]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2014-08-15 06:04:54]

lgtm

[commit-bot: I haz the power, 2014-08-15 06:05:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hayato@chromium.org/448893003/60001

[commit-bot: I haz the power, 2014-08-15 06:28:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-08-15 06:31:19]

Committed patchset #4 (60001) as 180332

[kozyatinskiy1, 2014-09-18 13:26:24]

On 2014/08/15 06:31:19, I haz the power (commit-bot) wrote:
> Committed patchset #4 (60001) as 180332

Hi Elliott. Could you please take a look on crash report in assertion that you
added: https://code.google.com/p/chromium/issues/detail?id=415549.