Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
458933002:
Fix ScriptPreprocessor leak in inspector test case of LayoutTest. (Closed)
|
Created:
6 years, 4 months ago by Miyoung Shin(g) Modified:
6 years, 4 months ago CC:
blink-reviews, vsevik+blink_chromium.org, caseq+blink_chromium.org, arv+blink, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, loislo+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, blink-reviews-bindings_chromium.org, devtools-reviews_chromium.org, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix ScriptPreprocessor leak in inspector test case of LayoutTest.
We need to clear preprocessor when disabling inspector.
LeakExpectations : remove fixed bugs of inspector
BUG=301515, 364408, 376188
R=haraken@chromium.org
R=kouhei@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180068
Patch Set 1 #Patch Set 2 : including changes to LeakExpectation #Patch Set 3 : rebase #
Messages
Total messages: 17 (0 generated)
Can we get rid of some entries from LeakExpectations?
inspector lgtm
On 2014/08/11 08:00:48, haraken wrote: > Can we get rid of some entries from LeakExpectations? It seems that the current LeakExpectations for the inspector tests contains already fixed cases. In my analysis, it's related to executing script in isolated world and fixed by Kohei's patch. But I found the problem to remain the ScriptPreprocessor class's reference count if I change to simplify the test case of inspector. It would be a potential leak. I had a discussion with Mr.Kohie and he agreed with using the number of 364408. After this patch, I have a plan to get rid of the test cases of inpector in LeakExpectations
On 2014/08/11 13:03:00, Miyoung Shin(g) wrote: > On 2014/08/11 08:00:48, haraken wrote: > > Can we get rid of some entries from LeakExpectations? > > It seems that the current LeakExpectations for the inspector tests contains > already fixed cases. > In my analysis, it's related to executing script in isolated world and fixed by > Kohei's patch. > But I found the problem to remain the ScriptPreprocessor class's reference count > if I change to simplify the test case of inspector. > It would be a potential leak. > I had a discussion with Mr.Kohie and he agreed with using the number of 364408. > After this patch, I have a plan to get rid of the test cases of inpector in > LeakExpectations changes lgtm. You can include changes to LeakExpectation in this CL.
On 2014/08/11 13:20:06, kouhei wrote: > On 2014/08/11 13:03:00, Miyoung Shin(g) wrote: > > On 2014/08/11 08:00:48, haraken wrote: > > > Can we get rid of some entries from LeakExpectations? > > > > It seems that the current LeakExpectations for the inspector tests contains > > already fixed cases. > > In my analysis, it's related to executing script in isolated world and fixed > by > > Kohei's patch. > > But I found the problem to remain the ScriptPreprocessor class's reference > count > > if I change to simplify the test case of inspector. > > It would be a potential leak. > > I had a discussion with Mr.Kohie and he agreed with using the number of > 364408. > > After this patch, I have a plan to get rid of the test cases of inpector in > > LeakExpectations > > changes lgtm. You can include changes to LeakExpectation in this CL. Yes, we want to keep the CL and LeakExpectation in sync, so please include the change into this CL. LGTM.
On 2014/08/11 13:36:45, haraken wrote: > On 2014/08/11 13:20:06, kouhei wrote: > > On 2014/08/11 13:03:00, Miyoung Shin(g) wrote: > > > On 2014/08/11 08:00:48, haraken wrote: > > > > Can we get rid of some entries from LeakExpectations? > > > > > > It seems that the current LeakExpectations for the inspector tests contains > > > already fixed cases. > > > In my analysis, it's related to executing script in isolated world and fixed > > by > > > Kohei's patch. > > > But I found the problem to remain the ScriptPreprocessor class's reference > > count > > > if I change to simplify the test case of inspector. > > > It would be a potential leak. > > > I had a discussion with Mr.Kohie and he agreed with using the number of > > 364408. > > > After this patch, I have a plan to get rid of the test cases of inpector in > > > LeakExpectations > > > > changes lgtm. You can include changes to LeakExpectation in this CL. > > Yes, we want to keep the CL and LeakExpectation in sync, so please include the > change into this CL. LGTM. I've included changes to LeakExpectation in the CL. please review Patch set 2
LGTM
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/myid.o.shin@gmail.com/458933002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for LayoutTests/LeakExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/LeakExpectations Hunk #1 succeeded at 38 (offset 3 lines). Hunk #2 succeeded at 58 (offset 3 lines). Hunk #3 FAILED at 78. 1 out of 3 hunks FAILED -- saving rejects to file LayoutTests/LeakExpectations.rej Patch: LayoutTests/LeakExpectations Index: LayoutTests/LeakExpectations diff --git a/LayoutTests/LeakExpectations b/LayoutTests/LeakExpectations index 22ba30df18db201b7b319be5792af8c0a9a958c0..ca3b8f8293cf3efcb9723fde329534a037e0a975 100644 --- a/LayoutTests/LeakExpectations +++ b/LayoutTests/LeakExpectations @@ -35,7 +35,6 @@ crbug.com/371264 virtual/implsidepainting/inspector/timeline/timeline-window-fil # The leak detector doesn't wait for worker threads to completely clean up, so leaks are expected. crbug.com/301515 fast/workers/worker-terminate.html [ Leak ] -crbug.com/301515 inspector-protocol/debugger/debugger-terminate-dedicated-worker-while-paused.html [ Leak ] # ----------------------------------------------------------------- # Untriaged but known real leaks. @@ -56,12 +55,6 @@ crbug.com/398742 fast/workers/shared-worker-event-listener.html [ Leak ] crbug.com/398742 fast/workers/worker-crash-with-invalid-location.html [ Leak ] crbug.com/398742 http/tests/security/contentSecurityPolicy/1.1/child-src/worker-shared-allowed.html [ Leak ] -crbug.com/364408 inspector/extensions/extensions-audits-content-script.html [ Leak ] -crbug.com/364408 inspector/extensions/extensions-eval-content-script.html [ Leak ] -crbug.com/364408 inspector/extensions/extensions-reload.html [ Leak ] -crbug.com/364408 inspector/sources/debugger/debugger-script-preprocessor.html [ Leak ] -crbug.com/364408 inspector/sources/debugger/live-edit-breakpoints.html [ Leak Timeout ] - crbug.com/364411 fast/frames/location-observe-callback-crash.html [ Leak ] crbug.com/364417 editing/selection/selection-in-iframe-removed-crash.html [ Leak ] @@ -85,91 +78,3 @@ crbug.com/366455 svg/W3C-SVG-1.1/text-intro-01-t.svg [ Leak ] crbug.com/366455 svg/W3C-SVG-1.1/text-intro-04-t.svg [ Leak ] crbug.com/366455 svg/css/font-face-crash.html [ Leak ] crbug.com/366455 svg/custom/svg-fonts-fallback.xhtml [ Leak ] - -crbug.com/376188 http/tests/inspector/compiler-source-mapping-debug.html [ Leak ] -crbug.com/376188 http/tests/inspector/network/load-resource-when-paused.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/access-obsolete-frame.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/continueToLocation.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/debugger-evaluate-in-worker-while-pause-in-page.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/debugger-setVariableValue.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/getStepInPositions.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/postMessage-on-pause.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/setScriptSource.html [ Leak ] -crbug.com/376188 inspector-protocol/debugger/updateCallFrameScopes.html [ Leak ] -crbug.com/376188 inspector-protocol/loading-iframe-document-node.html [ Leak ] -crbug.com/376188 inspector-protocol/runtime/runtime-getProperties.html [ Leak ] -crbug.com/376188 inspector/console/console-api-on-call-frame.html [ Leak ] -crbug.com/376188 inspector/console/console-error-on-call-frame.html [ Leak ] -crbug.com/376188 inspector/console/console-format.html [ Leak ] -crbug.com/376188 inspector/console/console-native-function.html [ Leak ] -crbug.com/376188 inspector/console/console-save-to-temp-var.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-and-framework-black-box.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-eval.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-middle-run.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-mutation-observer.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-scopes.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack-xhrs.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/async-callstack.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/callstack-placards-discarded.html [ Leak Timeout ] -crbug.com/376188 inspector/sources/debugger/copy-stack-trace.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/custom-element-lifecycle-events.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debug-console-command.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debug-inlined-scripts-fragment-id.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debug-inlined-scripts.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-activation-crash2.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-change-variable.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-command-line-api.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-completions-on-call-frame.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-eval-on-call-frame.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-eval-while-paused.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-expand-scope.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-no-nested-pause.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-no-pause-on-antibreakpoint.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-in-eval-script.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-in-internal.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-blocked-event-handler.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-blocked-script-injection.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-blocked-script-url.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-debugger-statement.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-exception.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-on-failed-assertion.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-pause-with-overrides.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-proto-property.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-reload-on-pause.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-return-value.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-set-breakpoint-regex.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-step-in.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-step-into-custom-element-callbacks.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-step-into-event-listener.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-step-out.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-step-over.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/debugger-suspend-active-dom-objects.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/dom-breakpoints.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/dynamic-scripts-breakpoints.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/eval-on-pause-blocked.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/event-listener-breakpoints.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/frameworks-dom-xhr-event-breakpoints.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/frameworks-skip-step-in.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/frameworks-steppings.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/frameworks-with-async-callstack.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/function-details.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/function-display-name-call-stack.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/js-with-inline-stylesheets.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/live-edit-no-reveal.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/live-edit.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/mutation-observer-suspend-while-paused.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/pause-in-inline-script.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/pause-in-internal-script.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/properties-special.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/reveal-execution-line.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/reveal-not-skipped.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/script-formatter-breakpoints-1.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/script-formatter-breakpoints-2.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/script-formatter-breakpoints-3.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/selected-call-frame-after-formatting-source.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/set-breakpoint.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/skip-pauses-until-reload.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/skip-stack-frames-steps.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/step-through-event-listeners.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/watch-expressions-panel-switch.html [ Leak ] -crbug.com/376188 inspector/sources/debugger/xhr-breakpoints.html [ Leak ]
The CQ bit was checked by myid.o.shin@gmail.com
The CQ bit was unchecked by myid.o.shin@gmail.com
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/myid.o.shin@gmail.com/458933002/40001
Message was sent while issue was closed.
Change committed as 180068 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
