If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 50

Status

()

Core
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Tomcat, Assigned: xidorn)

Tracking

({intermittent-failure, mlk})

unspecified
mozilla50
intermittent-failure, mlk
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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)
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=259215eef842ec7d2e7266e0052ede53c07a853a&bugfiler&filter-tier=1&filter-searchStr=8fded4bc4ae7b5f380fa21318ff0a7fdb81baac3&tochange=b7188125e7317d13c22c3973478d57b866c1662d
Depends on: 1160847
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 2

a year ago
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...
(Assignee)

Updated

a year ago
Blocks: 1160847
No longer depends on: 1160847
(Assignee)

Comment 3

a year ago
Cannot reproduce this issue on try :|
https://treeherder.mozilla.org/#/jobs?repo=try&revision=327bb441af7d

Comment 4

a year ago
29 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 24
* autoland: 4
* fx-team: 1

Platform breakdown:
* windows7-32: 29

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-06-29&endday=2016-06-29&tree=all
(Assignee)

Comment 5

a year ago
Hmm, it doesn't happen in Windows 7 VM, but does happen in Windows 7. Interesting...
(Assignee)

Comment 6

a year ago
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...
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Comment 8

a year ago
I have an idea about how to diagnose. Will try tomorrow...

Comment 9

a year ago
29 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 19
* fx-team: 6
* autoland: 3
* mozilla-central: 1

Platform breakdown:
* windows7-32: 29

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-06-30&endday=2016-06-30&tree=all
(Assignee)

Comment 10

a year ago
With my patch for bug 1283337, I eventually get this:
> TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray<gfxSkipChars::SkippedRange>)
(Assignee)

Comment 11

a year ago
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.
32 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 17
* autoland: 8
* fx-team: 6
* mozilla-central: 1

Platform breakdown:
* windows7-32: 32

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-07-01&endday=2016-07-01&tree=all
(Assignee)

Comment 13

a year ago
(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
(Assignee)

Comment 14

a year ago
Created attachment 8767534 [details]
Bug 1283106 part 1 - Guard all methods of gfxSkipCharsIterator with check on mSkipChars.

Review commit: https://reviewboard.mozilla.org/r/61984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61984/
Attachment #8767534 - Flags: review?(jfkthame)
Attachment #8767535 - Flags: review?(jfkthame)
(Assignee)

Comment 15

a year ago
Created attachment 8767535 [details]
Bug 1283106 part 2 - Return uninitialized gfxSkipCharsIterator rather than static const for failure.

Review commit: https://reviewboard.mozilla.org/r/61986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61986/
(Assignee)

Comment 16

a year ago
Let's see whether it works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=904ff49c9249
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
114 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 64
* autoland: 24
* fx-team: 21
* mozilla-central: 3
* try: 2

Platform breakdown:
* windows7-32: 114

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-06-27&endday=2016-07-03&tree=all
(Assignee)

Comment 18

a year ago
So the patch looks good.
(Assignee)

Comment 19

a year ago
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?
31 automation job failures were associated with this bug yesterday.

Repository breakdown:
* fx-team: 10
* mozilla-inbound: 9
* autoland: 9
* try: 2
* mozilla-central: 1

Platform breakdown:
* windows7-32: 31

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-07-04&endday=2016-07-04&tree=all
(Assignee)

Comment 22

a year ago
Created attachment 8767838 [details]
Bug 1283106 - Put empty skip chars in gfxPlatform singleton rather than static local variable.

Review commit: https://reviewboard.mozilla.org/r/62226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62226/
Attachment #8767838 - Flags: review?(jfkthame)
(Assignee)

Updated

a year ago
Attachment #8767534 - Attachment is obsolete: true
Attachment #8767534 - Flags: review?(jfkthame)
(Assignee)

Updated

a year ago
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+
(Assignee)

Comment 24

a year ago
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.
41 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 18
* autoland: 12
* fx-team: 8
* mozilla-central: 2
* try: 1

Platform breakdown:
* windows7-32: 41

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-07-05&endday=2016-07-05&tree=all

Comment 26

a year ago
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
(Reporter)

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/78d2fb74e22f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
31 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 20
* fx-team: 6
* autoland: 3
* try: 1
* mozilla-central: 1

Platform breakdown:
* windows7-32: 30
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-07-06&endday=2016-07-06&tree=all
111 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 53
* fx-team: 24
* autoland: 24
* try: 5
* mozilla-central: 4
* ash: 1

Platform breakdown:
* windows7-32: 110
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1283106&startday=2016-07-04&endday=2016-07-10&tree=all
You need to log in before you can comment on or make changes to this bug.