Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
415743005:
Move ScopedStyleResolver and ScopedStyleTree to oilpan's heap (Closed)
|
Created:
6 years, 5 months ago by haraken Modified:
6 years, 5 months ago CC:
blink-reviews, blink-reviews-css, eae+blinkwatch, ed+blinkwatch_opera.com, blink-reviews-dom_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, rune+blink, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionMove ScopedStyleResolver and ScopedStyleTree to oilpan's heap
In oilpan, TreeScope, all Nodes in the TreeScope, StyleEngine, StyleResolver and ScopedStyleTree live and die together. Thus we can make raw pointers in ScopedStyleTree Members. Also we can remove clearScopedStyleResolver() from ScopedStyleTree's destructor, because the TreeScope and ScopedStyleTree live and die together.
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178817
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Messages
Total messages: 30 (0 generated)
PTAL
I noticed that this CL is not enough to fix crashes happening on oilpan bots. Please hold on review.
This fixes a lot of outstanding crashes in oilpan bots. PTAL. In a follow-up CL, kochi-san is planning to remove ScopedStyleTree::m_authorStyles and ScopedStyleTree::m_cache.
blind lgtm (defer real reviews to oilpan experts) Thanks for fixing my breakage.
tkent-san: Would you take a look?
lgtm https://codereview.chromium.org/415743005/diff/40001/Source/core/css/resolver... File Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/415743005/diff/40001/Source/core/css/resolver... Source/core/css/resolver/ScopedStyleResolver.cpp:80: resolver->processScopedRules(ruleSet, cssSheet, m_scope->rootNode()); nit: We prefer a reference. treeScope().rootNode()
https://codereview.chromium.org/415743005/diff/40001/Source/core/css/resolver... File Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/415743005/diff/40001/Source/core/css/resolver... Source/core/css/resolver/ScopedStyleResolver.cpp:80: resolver->processScopedRules(ruleSet, cssSheet, m_scope->rootNode()); On 2014/07/24 04:30:09, tkent wrote: > nit: We prefer a reference. treeScope().rootNode() Done.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/415743005/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) android_blink_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16842) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18639) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32500) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/37467)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16850)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/415743005/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
Message was sent while issue was closed.
Change committed as 178817
Message was sent while issue was closed.
Seems like this caused a number of crashes, https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa...
Message was sent while issue was closed.
On 2014/07/24 11:23:16, sof wrote: > Seems like this caused a number of crashes, > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... Should we just revert it for now or is there anyone looking at a fix?
Message was sent while issue was closed.
On 2014/07/24 11:29:30, wibling-chromium wrote: > On 2014/07/24 11:23:16, sof wrote: > > Seems like this caused a number of crashes, > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > Should we just revert it for now or is there anyone looking at a fix? I'm looking. Reverting this CL will cause other crashes in layout tests... Also some CL in r178791 - r178820 caused a lot of failures of style-related tests. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/...
Message was sent while issue was closed.
On 2014/07/24 11:32:34, haraken wrote: > On 2014/07/24 11:29:30, wibling-chromium wrote: > > On 2014/07/24 11:23:16, sof wrote: > > > Seems like this caused a number of crashes, > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > Should we just revert it for now or is there anyone looking at a fix? > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > Also some CL in r178791 - r178820 caused a lot of failures of style-related > tests. > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... Okay. I won't do anything rash then :)
Message was sent while issue was closed.
On 2014/07/24 11:32:34, haraken wrote: > On 2014/07/24 11:29:30, wibling-chromium wrote: > > On 2014/07/24 11:23:16, sof wrote: > > > Seems like this caused a number of crashes, > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > Should we just revert it for now or is there anyone looking at a fix? > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > Also some CL in r178791 - r178820 caused a lot of failures of style-related > tests. > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... It seems like the same set (but the stacks aren't of much use atm on that platform.) This CL is in that range :)
Message was sent while issue was closed.
On 2014/07/24 11:36:26, sof wrote: > On 2014/07/24 11:32:34, haraken wrote: > > On 2014/07/24 11:29:30, wibling-chromium wrote: > > > On 2014/07/24 11:23:16, sof wrote: > > > > Seems like this caused a number of crashes, > > > > > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > > > Should we just revert it for now or is there anyone looking at a fix? > > > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > > > Also some CL in r178791 - r178820 caused a lot of failures of style-related > > tests. > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > It seems like the same set (but the stacks aren't of much use atm on that > platform.) This CL is in that range :) Yes, this CL is in the range, but a lot of things have been broken since the original CL (r178795) was landed. It's possible that they are crashes introduced in the original CL.
Message was sent while issue was closed.
https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... File Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... Source/core/css/resolver/ScopedStyleResolver.cpp:87: if (contents->hasOneClient() || visitedSharedStyleSheetContents.add(contents).isNewEntry) I noticed that this condition doesn't work as expected in oilpan builds. Whether the contents has one client or not depends on GC timing... I'm investigating how to update the condition.
Message was sent while issue was closed.
On 2014/07/24 11:45:40, haraken wrote: > On 2014/07/24 11:36:26, sof wrote: > > On 2014/07/24 11:32:34, haraken wrote: > > > On 2014/07/24 11:29:30, wibling-chromium wrote: > > > > On 2014/07/24 11:23:16, sof wrote: > > > > > Seems like this caused a number of crashes, > > > > > > > > > > > > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > > > > > Should we just revert it for now or is there anyone looking at a fix? > > > > > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > > > > > Also some CL in r178791 - r178820 caused a lot of failures of style-related > > > tests. > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > It seems like the same set (but the stacks aren't of much use atm on that > > platform.) This CL is in that range :) > > Yes, this CL is in the range, but a lot of things have been broken since the > original CL (r178795) was landed. It's possible that they are crashes introduced > in the original CL. I suspect that is the case. I tried reverting 178795, 178800, and 178817 and there are significantly less tests failing (25 after reverting and 68 without reverting) and no crashes after reverting.
Message was sent while issue was closed.
On 2014/07/24 12:10:08, wibling-chromium wrote: > On 2014/07/24 11:45:40, haraken wrote: > > On 2014/07/24 11:36:26, sof wrote: > > > On 2014/07/24 11:32:34, haraken wrote: > > > > On 2014/07/24 11:29:30, wibling-chromium wrote: > > > > > On 2014/07/24 11:23:16, sof wrote: > > > > > > Seems like this caused a number of crashes, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > > > > > > > Should we just revert it for now or is there anyone looking at a fix? > > > > > > > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > > > > > > > Also some CL in r178791 - r178820 caused a lot of failures of > style-related > > > > tests. > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > It seems like the same set (but the stacks aren't of much use atm on that > > > platform.) This CL is in that range :) > > > > Yes, this CL is in the range, but a lot of things have been broken since the > > original CL (r178795) was landed. It's possible that they are crashes > introduced > > in the original CL. > > I suspect that is the case. I tried reverting 178795, 178800, and 178817 and > there are significantly less tests failing (25 after reverting and 68 without > reverting) and no crashes after reverting. I think I found the cause. Will upload a CL shortly.
Message was sent while issue was closed.
On 2014/07/24 11:58:50, haraken wrote: > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > File Source/core/css/resolver/ScopedStyleResolver.cpp (right): > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > Source/core/css/resolver/ScopedStyleResolver.cpp:87: if > (contents->hasOneClient() || > visitedSharedStyleSheetContents.add(contents).isNewEntry) > > I noticed that this condition doesn't work as expected in oilpan builds. Whether > the contents has one client or not depends on GC timing... I'm investigating how > to update the condition. Yes, when fast/media/mq-js-media-except-03.html fails, there's one completed client left, but no ruleset. But I don't understand why the weak member hasn't been cleared..assuming there are no other code paths that explicitly unregister that doesn't run with Oilpan enabled. Can't see any, but don't know the code. (FTR, I verified that r178815 wasn't involved somehow.)
Message was sent while issue was closed.
On 2014/07/24 12:18:03, sof wrote: > On 2014/07/24 11:58:50, haraken wrote: > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > File Source/core/css/resolver/ScopedStyleResolver.cpp (right): > > > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > Source/core/css/resolver/ScopedStyleResolver.cpp:87: if > > (contents->hasOneClient() || > > visitedSharedStyleSheetContents.add(contents).isNewEntry) > > > > I noticed that this condition doesn't work as expected in oilpan builds. > Whether > > the contents has one client or not depends on GC timing... I'm investigating > how > > to update the condition. > > Yes, when fast/media/mq-js-media-except-03.html fails, there's one completed > client left, but no ruleset. But I don't understand why the weak member hasn't > been cleared..assuming there are no other code paths that explicitly unregister > that doesn't run with Oilpan enabled. Can't see any, but don't know the code. > > (FTR, I verified that r178815 wasn't involved somehow.) The reason would be that we don't have code to clear ScopedStyleResolvers when clearRuleSet() is called. When clearRuleSet() is called, normal StyleResolvers are cleared but ScopedStyleResolvers are not cleared. I uploaded a fix here: https://codereview.chromium.org/419513002/
Message was sent while issue was closed.
On 2014/07/24 12:22:30, haraken wrote: > On 2014/07/24 12:18:03, sof wrote: > > On 2014/07/24 11:58:50, haraken wrote: > > > > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > > File Source/core/css/resolver/ScopedStyleResolver.cpp (right): > > > > > > > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > > Source/core/css/resolver/ScopedStyleResolver.cpp:87: if > > > (contents->hasOneClient() || > > > visitedSharedStyleSheetContents.add(contents).isNewEntry) > > > > > > I noticed that this condition doesn't work as expected in oilpan builds. > > Whether > > > the contents has one client or not depends on GC timing... I'm investigating > > how > > > to update the condition. > > > > Yes, when fast/media/mq-js-media-except-03.html fails, there's one completed > > client left, but no ruleset. But I don't understand why the weak member hasn't > > been cleared..assuming there are no other code paths that explicitly > unregister > > that doesn't run with Oilpan enabled. Can't see any, but don't know the code. > > > > (FTR, I verified that r178815 wasn't involved somehow.) > > The reason would be that we don't have code to clear ScopedStyleResolvers when > clearRuleSet() is called. When clearRuleSet() is called, normal StyleResolvers > are cleared but ScopedStyleResolvers are not cleared. > > I uploaded a fix here: https://codereview.chromium.org/419513002/ So I guess this issue is not related to GC timing.
Message was sent while issue was closed.
On 2014/07/24 12:23:13, haraken wrote: > On 2014/07/24 12:22:30, haraken wrote: > > On 2014/07/24 12:18:03, sof wrote: > > > On 2014/07/24 11:58:50, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > > > File Source/core/css/resolver/ScopedStyleResolver.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/415743005/diff/80001/Source/core/css/resolver... > > > > Source/core/css/resolver/ScopedStyleResolver.cpp:87: if > > > > (contents->hasOneClient() || > > > > visitedSharedStyleSheetContents.add(contents).isNewEntry) > > > > > > > > I noticed that this condition doesn't work as expected in oilpan builds. > > > Whether > > > > the contents has one client or not depends on GC timing... I'm > investigating > > > how > > > > to update the condition. > > > > > > Yes, when fast/media/mq-js-media-except-03.html fails, there's one completed > > > client left, but no ruleset. But I don't understand why the weak member > hasn't > > > been cleared..assuming there are no other code paths that explicitly > > unregister > > > that doesn't run with Oilpan enabled. Can't see any, but don't know the > code. > > > > > > (FTR, I verified that r178815 wasn't involved somehow.) > > > > The reason would be that we don't have code to clear ScopedStyleResolvers when > > clearRuleSet() is called. When clearRuleSet() is called, normal StyleResolvers > > are cleared but ScopedStyleResolvers are not cleared. > > > > I uploaded a fix here: https://codereview.chromium.org/419513002/ > > So I guess this issue is not related to GC timing. Great fix, regardless :-) I'll watch the review for any explanations of why. The tests that previously failed locally, now pass.
Message was sent while issue was closed.
On 2014/07/24 12:10:08, wibling-chromium wrote: > On 2014/07/24 11:45:40, haraken wrote: > > On 2014/07/24 11:36:26, sof wrote: > > > On 2014/07/24 11:32:34, haraken wrote: > > > > On 2014/07/24 11:29:30, wibling-chromium wrote: > > > > > On 2014/07/24 11:23:16, sof wrote: > > > > > > Seems like this caused a number of crashes, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > > > > > > > > > Should we just revert it for now or is there anyone looking at a fix? > > > > > > > > I'm looking. Reverting this CL will cause other crashes in layout tests... > > > > > > > > Also some CL in r178791 - r178820 caused a lot of failures of > style-related > > > > tests. > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > It seems like the same set (but the stacks aren't of much use atm on that > > > platform.) This CL is in that range :) > > > > Yes, this CL is in the range, but a lot of things have been broken since the > > original CL (r178795) was landed. It's possible that they are crashes > introduced > > in the original CL. > > I suspect that is the case. I tried reverting 178795, 178800, and 178817 and > there are significantly less tests failing (25 after reverting and 68 without > reverting) and no crashes after reverting. That matches up fairly well with where we are now after r178840; some tests fail, but no crashes -- https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
