Closed Bug 1283106 Opened 8 years ago Closed 8 years ago

Intermittent leakcheck | default process: 4 bytes leaked (nsTArray_base)

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: cbook, Assigned: xidorn)

References

(
URL
)

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(1 file, 2 obsolete files)

Bug 1283106 - Put empty skip chars in gfxPlatform singleton rather than static local variable.
58 bytes, text/x-review-board-request
jfkthame
: review+
Details 
https://treeherder.mozilla.org/logviewer.html#?job_id=30889441&repo=mozilla-inbound#L19220

 05:55:46 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray_base)
i totally don't understand how this could happen... The patches in bug 1160847 did not add any nsTArray uses out of stack at all...
Blocks: 1160847
No longer depends on: 1160847
Hmm, it doesn't happen in Windows 7 VM, but does happen in Windows 7. Interesting...
Still have no idea what's going wrong there...

Based on https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c93cff3cbf I guess this is from bug 1160847 part 3, but I have no idea how that patch can lead to leak of nsTArray...
But that failure only appears once for the try push in comment 3 as well... It is hard to say whether part 3 really causes that, then.
I have an idea about how to diagnose. Will try tomorrow...
With my patch for bug 1283337, I eventually get this:
> TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray<gfxSkipChars::SkippedRange>)
The only place who owns a nsTArray<gfxSkipChars::SkippedRange> is a member field of gfxSkipChars. gfxSkipChars is used in local variable twice, local static variable twice, and can be owned by gfxTextRun. If gfxTextRun leaks, there should be another observable leak of nsTArray<GlyphRun>, so it's apparently not that case. So my guess is one of the static local is somehow initialized.

We should either somehow get rid of that static local, or make gfxSkipChars not own a nsTArray. Alternatively, if we can mark an allocation static and not mark it as a leak, that would probably be the best.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> With my patch for bug 1283337, I eventually get this:
> > TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray<gfxSkipChars::SkippedRange>)

FWIW, the try push is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7374c4c6fd0d
Let's see whether it works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=904ff49c9249
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
So the patch looks good.
I mean, the try result looks good.
Great diagnosis!

IIRC the performance of gfxSkipCharsIterator is pretty critical as it's on some hot code paths, so adding an extra test+branch to some of its methods seems unfortunate if we can avoid it. Currently, we never call these methods on an uninitialized iterator and so the test for mSkipChars is not needed.

The leak here is unimportant, given that it's just a single static const that we're creating as a fallback to handle an apparent error case; so if we could somehow just avoid reporting it that would be preferable (as per comment 11). But I don't know if we have a way to do that.

Another possibility would be to have an empty gfxSkipChars that's owned by something (e.g. the gfxPlatform singleton?) that gets torn down during shutdown, and use that as the basis for the error-case iterators. AFAICS that would allow us to do things like

    return gfxSkipCharsIterator(gfxPlatform::GetPlatform()->EmptySkipChars(), 0);

in these cases, and avoid the reported leak.

The other thing I'm wondering is why bug 1160847 made this start happening so frequently. Of the two places where a static const gfxSkipChars may get created, the second one is clearly an error case (and should be issuing an NS_ERROR if we hit it), so presumably what we're seeing here is the first case, where we didn't construct an expected textrun for some reason. The comment there says "This is bad." Can we figure out what's going on here, and why it has suddenly become a common occurrence? Does it indicate a flaw in the patches from bug 1160847, or an old assumption that is no longer valid?
Attachment #8767534 - Attachment is obsolete: true
Attachment #8767534 - Flags: review?(jfkthame)
Attachment #8767535 - Attachment is obsolete: true
Attachment #8767535 - Flags: review?(jfkthame)
Comment on attachment 8767838 [details]
Bug 1283106 - Put empty skip chars in gfxPlatform singleton rather than static local variable.

https://reviewboard.mozilla.org/r/62226/#review58980

Yeah, this looks fine. I'd still like to understand why this suddenly became an issue, though; what specific change in behavior made us start hitting the missing-textrun case, and can/should we do anything about it besides wallpapering over the "leak" here?
Attachment #8767838 - Flags: review?(jfkthame) → review+
In this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4570de259018&selectedJob=23346521 I make it print some information when that happens.

This happens in this subtree:
>  Block(description)(0)@215c46f8 next=24f9ac38 {360,60,17340,3240} vis-overflow=-120,0,17460,3240 scr-overflow=0,0,17340,3240 [state=000f160000d01020] [content=1ca4dce0] [sc=1dde7d38]<
>    line 1e460438: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} <
>      TextBox(label)(0)[value=Hint]@2120f218 next=216e0598 {0,0,0,0} [state=000d060000000402] [content=1eb48ce0] [sc=1926d0e8]
>    >
>    line 1f5c2658: count=1 state=inline,dirty,prevmargindirty,not impacted,wrapped,before:nobr,after:nobr[0x163] {0,0,16920,1080} vis-overflow=0,0,17220,1080 scr-overflow=0,0,16920,1080 <
>      Text(1)": You can customize Nightly to work the way you "@216e0598 next=1f5c2e50 next-in-flow=1f5c2e50 {1440,90,15480,900} vis-overflow=-60,0,15840,900 scr-overflow=0,0,15480,900 [state=000d004081400000] [content=1eb48d30] [sc=1e427d00:-moz-text] [run=0][0,48,F]
>    >
>    line 198d5760: count=1 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x121] {0,1080,15360,1080} vis-overflow=-60,1080,15720,1080 scr-overflow=0,1080,15360,1080 <
>      Text(1)"do. Simply drag any of the above to the menu or "@1f5c2e50 next=1f5c2db8 prev-in-flow=216e0598 next-in-flow=1f5c2db8 {0,1170,15360,900} vis-overflow=-60,0,15720,900 scr-overflow=0,0,15360,900 [state=000d004091600004] [content=1eb48d30] [sc=1e427d00:-moz-text] [run=0][48,48,F]
>    >
>    line 1951e4c0: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x101] {0,2160,15060,1080} vis-overflow=-120,2160,15180,1080 scr-overflow=0,2160,15060,1080 <
>      Text(1)"toolbar. "@1f5c2db8 next=2120f610 prev-in-flow=1f5c2e50 {0,2250,2640,900} vis-overflow=-120,0,2880,900 scr-overflow=0,0,2640,900 [state=000d004090200004] [content=1eb48d30] [sc=1e427d00:-moz-text] [run=0][96,9,T]
>      TextBox(label)(2)[value=Learn more]@2120f610 next=19878010 {0,0,0,0} [state=000d060000000402] [content=1eb48d80] [sc=1e4fe668]
>    >
>    line 1f5c28b8: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} vis-overflow=6180,2250,9060,900 scr-overflow=6240,2250,8820,900 <
>      Text(3)" about customizing Nightly."@19878010 {6240,2250,8820,900} vis-overflow=-60,0,9060,900 scr-overflow=0,0,8820,900 [state=000d0040a0400000] [content=1eb48dd0] [sc=1e427d00:-moz-text] [run=13a91680][0,27,T]
>    >
> > 

The failing frame is the text frame containing "toolbar.". I don't see how this could be related to 

The text is added here: https://dxr.mozilla.org/mozilla-central/rev/c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5/browser/components/customizableui/CustomizeMode.jsm#696-703

The stack is:
> #01: nsTextFrame::RecomputeOverflow(nsIFrame *) [layout/generic/nsTextFrame.cpp:9354]
> #02: nsTextFrame::ComputeCustomOverflow(nsOverflowAreas &) [layout/generic/nsTextFrame.cpp:9784]
> #03: nsIFrame::UpdateOverflow() [layout/generic/nsFrame.cpp:5985]
> #04: mozilla::OverflowChangedTracker::Flush() [layout/base/RestyleTracker.h:128]
> #05: mozilla::RestyleManager::EndProcessingRestyles() [layout/base/RestyleManager.cpp:1853]
> #06: mozilla::RestyleManager::ProcessRestyles(mozilla::RestyleTracker &) [obj-firefox/dist/include/mozilla/RestyleManager.h:536]
> #07: mozilla::RestyleManager::ProcessPendingRestyles() [layout/base/RestyleManager.cpp:1809]
> #08: PresShell::FlushPendingNotifications(mozFlushType) [layout/base/nsPresShell.cpp:4001]
> #09: nsDocument::FlushPendingNotifications(mozFlushType) [dom/base/nsDocument.cpp:8404]
> #10: nsComputedDOMStyle::UpdateCurrentStyleSources(bool) [layout/style/nsComputedDOMStyle.cpp:639]
> #11: nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const &,mozilla::ErrorResult &) [layout/style/nsComputedDOMStyle.cpp:797]
> #12: nsComputedDOMStyle::GetPropertyValue(nsAString_internal const &,nsAString_internal &) [layout/style/nsComputedDOMStyle.cpp:379]
> #13: nsComputedDOMStyle::GetPropertyValue(nsCSSProperty,nsAString_internal &) [layout/style/nsComputedDOMStyle.cpp:317]
> #14: mozilla::dom::CSS2PropertiesBinding::get_color [obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp:11927]
> #15: mozilla::dom::GenericBindingGetter(JSContext *,unsigned int,JS::Value *) [dom/bindings/BindingUtils.cpp:2718]
> #16: js::jit::DoCallNativeGetter(JSContext *,JS::Handle<JSFunction *>,JS::Handle<JSObject *>,JS::MutableHandle<JS::Value>) [js/src/jit/SharedIC.cpp:3165] 

And the JS stack is:
> 0 ToolbarIconColor.inferFromText() ["chrome://browser/content/browser.js":7839]
>     this = [object Object]
> 1 DevEdition._inferBrightness() ["chrome://browser/content/browser-devedition.js":60]
>     this = [object Object]
> 2 DevEdition.refreshBrowserDisplay() ["chrome://browser/content/browser-devedition.js":93]
>     this = [object Object]
> 3 DevEdition._toggleStyleSheet(deveditionThemeEnabled = false) ["chrome://browser/content/browser-devedition.js":109]
>     this = [object Object]
> 4 DevEdition.observe(subject = null, topic = "lightweight-theme-styling-update", data = "null") ["chrome://browser/content/browser-devedition.js":50]
>     this = [object Object]
> 5 _notifyWindows(aThemeData = null) ["resource://gre/modules/LightweightThemeManager.jsm":798]
> 6 this.LightweightThemeManager.themeChanged(aData = null) ["resource://gre/modules/LightweightThemeManager.jsm":311]
>     this = [object Object] 

It happens in browser/base/content/test/general/browser_devedition.js when it is unapplying themes.

But I still don't have idea based on those information. I tried to run only browser_devedition.js locally, but failed to reproduce this issue. I guess we need a test to trigger customization mode somehow first.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78d2fb74e22f
Put empty skip chars in gfxPlatform singleton rather than static local variable. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/78d2fb74e22f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: