Bugzilla

Reporter

Comment 1

•

8 years ago

I tried disabling browser_wyciwyg_urlbarCopying.js and the failures moved to browser_windowopen_reflows.js instead, so it appears this is a shutdown issue that just gets reported for the last test in the directory.

Summary: [e10s] WinXP/Win8 debug browser_wyciwyg_urlbarCopying.js | application crashed [@ mozilla::LinkedList<js::WeakMapBase>::~LinkedList<js::WeakMapBase>()] (Assertion failure: isEmpty(), at LinkedList.h:332) → [e10s] WinXP/Win8 debug browser/base/content/test/general application crashed [@ mozilla::LinkedList<js::WeakMapBase>::~LinkedList<js::WeakMapBase>()] (Assertion failure: isEmpty(), at LinkedList.h:332) on shutdown

Comment 2

•

8 years ago

This is one of our assertions that no JS is alive when we are shutting down. This typically indicates that some chrome code has a leak. Ideally, we'll be able to bisect to a chrome code change and the responsible party can use Andrew's CC and GC dumping tools to identify the edge causing the leak.

Flags: needinfo?(terrence)

Andrew McCreight [:mccr8]

Comment 3

•

8 years ago

Andrew volunteered to take a look once he's done investigating another release blocking leak.

Flags: needinfo?(continuation)

Comment 4

•

8 years ago

It seems bad that we crash when there's a leak. It will make investigating the leak much harder.

Comment 5

•

8 years ago

(In reply to Andrew McCreight [:mccr8] from comment #4)
> It seems bad that we crash when there's a leak. It will make investigating
> the leak much harder.

It seems that a bunch of type / object group sweeping stuff assumes that things are unreachable because the compartment is empty otherwise, so I think not crashing here would actually be extremely dangerous. Maybe we should just eagerly trigger dumping of the GC/CC logs if we get this assertion?

Reporter

Comment 6

•

8 years ago

Affects Win7 on Ash too.

Summary: [e10s] WinXP/Win8 debug browser/base/content/test/general application crashed [@ mozilla::LinkedList<js::WeakMapBase>::~LinkedList<js::WeakMapBase>()] (Assertion failure: isEmpty(), at LinkedList.h:332) on shutdown → [e10s] Windows debug browser/base/content/test/general application crashed [@ mozilla::LinkedList<js::WeakMapBase>::~LinkedList<js::WeakMapBase>()] (Assertion failure: isEmpty(), at LinkedList.h:332) on shutdown

Andrew McCreight [:mccr8]

Reporter

Updated

•

8 years ago

Whiteboard: [e10s-orangeblockers]

Comment 7

•

8 years ago

I'm not really going to be able to investigate a Windows-only leak.

Flags: needinfo?(continuation)

Boris Zbarsky [:bzbarsky]

Reporter

Comment 8

•

8 years ago

Try push with the assert commented out to see if we can get anything useful when it doesn't crash out from under us (hopefully):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c0d78533bcb&group_state=expanded

Comment 9

•

8 years ago

> so I think not crashing here would actually be extremely dangerous.

I would buy this argument if this were a release-mode crash, but it's not.  It's just a debug-only assertion failure.

Boris Zbarsky [:bzbarsky]

Updated

•

8 years ago

Flags: needinfo?(terrence)

Reporter

Comment 10

•

8 years ago

Looks like the TabChild.cpp assert & crash is still there. Also, we've got a leak log now.

https://treeherder.mozilla.org/logviewer.html#?job_id=18277938&repo=try

TEST-UNEXPECTED-FAIL | leakcheck | default process: 20763 bytes leaked (BackstagePass, CondVar, ConnectionHandle, ConsoleReportCollector, DataStorage, ...)

Comment 11

•

8 years ago

(In reply to Boris Zbarsky [:bz] from comment #9)
> > so I think not crashing here would actually be extremely dangerous.
> 
> I would buy this argument if this were a release-mode crash, but it's not. 
> It's just a debug-only assertion failure.

Fair point. The problem is that there are a ton of somewhat parallel assumptions about this spread out all over the place, so it's going to be a huge round of whack-a-mole if we just start removing assertions.

I think a better approach might be to detect the condition we care about earlier (maybe at the start of the GC, if possible), print some diagnostics, and skip the crashy bits; maybe even exit directly. I'll try to find some time to dig more tomorrow.

Updated

•

8 years ago

tracking-e10s: ? → +

Priority: -- → P1

Reporter

Comment 13

•

8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Looks like the TabChild.cpp assert & crash is still there. Also, we've got a
> leak log now.
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=18277938&repo=try
> 
> TEST-UNEXPECTED-FAIL | leakcheck | default process: 20763 bytes leaked
> (BackstagePass, CondVar, ConnectionHandle, ConsoleReportCollector,
> DataStorage, ...)

Jim, it seems that ^ is the real issue here (i.e. the IPC crash/assert still happen regardless of whether the GC assert is enabled or not). Per bug 1261396, jgriffin is looking to disable this directory soon so he can get Win7 debug mochitest-bc enabled in full production. Can we please bump up the priority on this?

status-firefox48: --- → affected

Component: JavaScript: GC → DOM: Content Processes

Flags: needinfo?(jmathies)

Comment 14

•

8 years ago

It's up as high as it can go without blocking e10s rollout. How many tests are we going to have to kill off to get bc running with this?

Flags: needinfo?(jmathies) → needinfo?(ryanvm)

Reporter

Comment 15

•

8 years ago

Roughly 300.

Flags: needinfo?(ryanvm)

Reporter

Comment 16

•

8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Roughly 300.

To further expand on that answer, it's roughly 300 test *files* in browser/base/content/test/general. Counting subtests, it's just over 10,000 total. It's the largest single directory of browser-chrome tests we run and accounts for about 26% of total browser-chrome coverage.

tracking-e10s: + → ?

Brad Lassey [:blassey] (use needinfo?)

Reporter

Updated

•

8 years ago

Flags: needinfo?(terrence)

Updated

•

8 years ago

tracking-e10s: ? → +

Reporter

Comment 17

•

8 years ago

Tests skipped per IRC discussion with jimm.

status-firefox46: --- → wontfix

status-firefox47: affected → wontfix

status-firefox48: affected → disabled

Keywords: leave-open

Whiteboard: [e10s-orangeblockers] → [e10s-orangeblockers][tests disabled on windows debug e10s]

Pulsebot

Comment 18

•

8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/74d3c0c9b49c

Reporter

Comment 19

•

8 years ago

FYI, this dropped us from 97% coverage on Windows debug m-bc to 76%.

Carsten Book [:Tomcat]

Comment 20

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/74d3c0c9b49c

Comment 21

•

8 years ago

Jim, is someone from the e10s team going to investigate here?

Flags: needinfo?(jmathies)

Comment 22

•

8 years ago

(In reply to Andrew Overholt [:overholt] from comment #21)
> Jim, is someone from the e10s team going to investigate here?

no not currently, we don't have anyone available.

Flags: needinfo?(jmathies)

Updated

•

8 years ago

Whiteboard: [e10s-orangeblockers][tests disabled on windows debug e10s] → [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-active

Updated

•

8 years ago

Whiteboard: [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-active → [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-fixlater

:Gijs (he/him)

Comment 23

•

8 years ago

Considering how much effort the frontend team put into enabling all these tests, and the number of tests disabled as a result of this issue, is "fix later" really the right prioritization?

Flags: needinfo?(overholt)

Comment 24

•

8 years ago

The situation wasn't clear to me, sorry.

I'll leave this up to Jim to find someone.

Flags: needinfo?(overholt) → needinfo?(jmathies)

Whiteboard: [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-fixlater → [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-followup-2016-04-19

Comment 25

•

8 years ago

We have debug test coverage for these tests on platforms other than Windows. The people who are best for looking at this now are all working on other top priority things related to OOMs or js/gc crashes so.. it's going to have to wait until we can find an owner interested in digging in.

Flags: needinfo?(jmathies)

Comment 26

•

8 years ago

I might be able to find someone on the Taipei platform team to look at this. They are all busy but might free up in a week or so.

Kan-Ru Chen [:kanru] (UTC+9)

Reporter

Comment 27

•

8 years ago

Nicolas, what are the odds that the |browser_wyciwyg_urlbarCopying.js | application crashed [@ mozilla::dom::TabChild::RecvShow(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const &,mozilla::dom::ShowInfo const &,mozilla::layers::TextureFactoryIdentifier const &,unsigned __int64 const &,mozilla::layout::PRenderFrameChild *,bool const &,nsSizeMode const &)]| part of this is related to the other graphics shutdown issues you've been looking at?

Flags: needinfo?(nical.bugzilla)

Nicolas Silva [:nical]

Comment 28

•

8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Nicolas, what are the odds that the |browser_wyciwyg_urlbarCopying.js |
> application crashed [@
> mozilla::dom::TabChild::RecvShow(mozilla::gfx::IntSizeTyped<mozilla::
> ScreenPixel> const &,mozilla::dom::ShowInfo const
> &,mozilla::layers::TextureFactoryIdentifier const &,unsigned __int64 const
> &,mozilla::layout::PRenderFrameChild *,bool const &,nsSizeMode const &)]|
> part of this is related to the other graphics shutdown issues you've been
> looking at?

I don't think this is related. PContent is mostly outside of gfx although there are a few interactions here and there. I can tell that there are things to figure out on that front, from the amount of PContent related logging on stdout when closing firefox. I filed bug 1250881 a little while ago.

Flags: needinfo?(nical.bugzilla)

Comment 29

•

8 years ago

Both failed tests browser_wyciwyg_urlbarCopying.js and browser_fullscreen-window-open.js have similar crash stacks. They both crash when a button is clicked and the click event handler tries to open a new window. It doesn't look like a shutdown crash though.

Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)

Updated

•

8 years ago

Whiteboard: [e10s-orangeblockers][tests disabled on windows debug e10s] btpp-followup-2016-04-19 → [e10s-orangeblockers][tests disabled on windows debug e10s]

Comment 30

•

8 years ago

We can not reproduce this issue in our lab (Windows 10 & Windows 8, VS2013 & VS2015).

For current m-c, try server doesn't fail too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d1dd9c6e24a (see mochitest bc4)

:Gijs (he/him)

Comment 31

•

8 years ago

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #30)
> We can not reproduce this issue in our lab (Windows 10 & Windows 8, VS2013 &
> VS2015).
> 
> For current m-c, try server doesn't fail too:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d1dd9c6e24a (see
> mochitest bc4)

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #29)
> Both failed tests browser_wyciwyg_urlbarCopying.js

This test, and about 45 others, were moved to a different dir as part of bug 1265055. It's quite possible that that hid the bug at issue here. It's also possible that the crash got fixed since we disabled the tests. It might be worth re-testing with a version of m-c that matches when this all got disabled to see if there is still crash work to be done or not. If you can reproduce with m-c at the time that this got disabled, you could then test if there's an app fix or if the bug went away because of the test movement by using the --appname mochitest switch to select a different (ie up-to-date) Firefox executable to use to run the tests.

In any case, it sounds like we should re-enable the tests if indeed they are consistently green on try. Ryan, does that sound right?

Flags: needinfo?(wpan)

Flags: needinfo?(ryanvm)

:Felipe Gomes (needinfo for replies!)

Reporter

Comment 32

•

8 years ago

Things are indeed much better than they were before. Maybe bug 1215265 helped with the crashes. However, there's still a Win8 permaleak :\

https://treeherder.mozilla.org/logviewer.html#?job_id=20113688&repo=try
TEST-INFO | leakcheck | default process: leaked 1 nsBaseDragService
TEST-INFO | leakcheck | default process: leaked 2 nsNativeDragTarget
TEST-INFO | leakcheck | default process: leaked 1 nsTArray_base
TEST-UNEXPECTED-FAIL | leakcheck | default process: 280 bytes leaked (nsBaseDragService, nsNativeDragTarget, nsTArray_base)

INFO -  Serial Numbers of Leaked Objects:
INFO -  1 @0000004E52B651A0 (1 references)
INFO -  allocation stack:
INFO -  #00: nsDragServiceConstructor [widget/windows/nsWidgetFactory.cpp:120]
INFO -  #01: nsComponentManagerImpl::CreateInstance(nsID const &,nsISupports *,nsID const &,void * *) [xpcom/components/nsComponentManager.cpp:1121]
INFO -  #02: nsComponentManagerImpl::GetService(nsID const &,nsID const &,void * *) [xpcom/components/nsComponentManager.cpp:1363]
INFO -  #03: nsNativeDragTarget::nsNativeDragTarget(nsIWidget *) [widget/windows/nsNativeDragTarget.cpp:48]
INFO -  #04: nsWindow::EnableDragDrop(bool) [widget/windows/nsWindow.cpp:3466]
INFO -  #05: nsView::AttachToTopLevelWidget(nsIWidget *) [view/nsView.cpp:721]
INFO -  #06: nsDocumentViewer::MakeWindow(nsSize const &,nsView *) [layout/base/nsDocumentViewer.cpp:2413]
INFO -  #07: nsDocumentViewer::InitInternal(nsIWidget *,nsISupports *,mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const &,bool,bool,bool) [layout/base/nsDocumentViewer.cpp:825]
INFO -  #08: nsDocumentViewer::Init(nsIWidget *,mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const &) [layout/base/nsDocumentViewer.cpp:619]
INFO -  #09: nsDocShell::SetupNewViewer(nsIContentViewer *) [docshell/base/nsDocShell.cpp:9261]
INFO -  #10: nsDocShell::Embed(nsIContentViewer *,char const *,nsISupports *) [docshell/base/nsDocShell.cpp:7152]
INFO -  #11: nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal *,nsIURI *,bool) [docshell/base/nsDocShell.cpp:7981]
INFO -  #12: nsWebShellWindow::Initialize(nsIXULWindow *,nsIXULWindow *,nsIURI *,int,int,bool,nsITabParent *,nsWidgetInitData &) [xpfe/appshell/nsWebShellWindow.cpp:219]
INFO -  #13: nsAppShellService::JustCreateTopWindow(nsIXULWindow *,nsIURI *,unsigned int,int,int,bool,nsITabParent *,nsWebShellWindow * *) [xpfe/appshell/nsAppShellService.cpp:746]
INFO -  #14: nsAppShellService::CreateTopLevelWindow(nsIXULWindow *,nsIURI *,unsigned int,int,int,nsITabParent *,nsIXULWindow * *) [xpfe/appshell/nsAppShellService.cpp:209]
INFO -  #15: nsAppStartup::CreateChromeWindow2(nsIWebBrowserChrome *,unsigned int,unsigned int,nsIURI *,nsITabParent *,bool *,nsIWebBrowserChrome * *) [toolkit/components/startup/nsAppStartup.cpp:669]
INFO -  #16: nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy *,char const *,char const *,char const *,bool,bool,bool,nsITabParent *,nsIArray *,float *,mozIDOMWindowProxy * *) [embedding/components/windowwatcher/nsWindowWatcher.cpp:834]
INFO -  #17: nsWindowWatcher::OpenWindow(mozIDOMWindowProxy *,char const *,char const *,char const *,nsISupports *,mozIDOMWindowProxy * *) [embedding/components/windowwatcher/nsWindowWatcher.cpp:369]
INFO -  #18: XPTC__InvokebyIndex
INFO -  #19: CallMethodHelper::Call() [js/xpconnect/src/XPCWrappedNative.cpp:1402]
INFO -  #20: XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [js/xpconnect/src/XPCWrappedNative.cpp:1367]
INFO -  #21: XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1128]
INFO -  #22: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:235]
INFO -  #23: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:480]
INFO -  #24: Interpret [js/src/vm/Interpreter.cpp:2831]
INFO -  #25: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:426]
INFO -  #26: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:498]
INFO -  #27: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:544]
INFO -  #28: JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2864]
INFO -  #29: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [js/xpconnect/src/XPCWrappedJSClass.cpp:1238]
INFO -  #30: nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [js/xpconnect/src/XPCWrappedJS.cpp:603]
INFO -  #31: PrepareAndDispatch [xpcom/reflect/xptcall/md/win32/xptcstubs_x86_64.cpp:174]
INFO -  #32: SharedStub
INFO -  #33: NS_CreateServicesFromCategory(char const *,nsISupports *,char const *,char16_t const *) [xpcom/components/nsCategoryManager.cpp:818]
INFO -  #34: nsXREDirProvider::DoStartup() [toolkit/xre/nsXREDirProvider.cpp:958]
INFO -  #35: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4217]
INFO -  #36: XREMain::XRE_main(int,char * * const,nsXREAppData const *) [toolkit/xre/nsAppRunner.cpp:4451]
INFO -  #37: XRE_main [toolkit/xre/nsAppRunner.cpp:4559]
INFO -  #38: do_main [browser/app/nsBrowserApp.cpp:220]
INFO -  #39: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:360]
INFO -  #40: wmain [toolkit/xre/nsWindowsWMain.cpp:138]
INFO -  #41: __scrt_common_main_seh [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:255]
INFO -  #42: KERNEL32.DLL + 0x167e
INFO -  #43: ntdll.dll + 0x1c3f1

If we could isolate the test(s) causing the leaks, we could re-enable the rest of the directory. I'd also be OK with closing this bug as WFM at this point and opening a new bug for the leak in question if it makes it easier from a tracking standpoint.

Flags: needinfo?(ryanvm)

Comment 33

•

8 years ago

(In reply to :Gijs Kruitbosch (back May 3rd) from comment #31)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #29)
> > Both failed tests browser_wyciwyg_urlbarCopying.js
> 
> This test, and about 45 others, were moved to a different dir as part of bug
> 1265055.

I'd also be really interested to know if the crash is gone by moving back the tests where they were, but using current m-c

Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)

Reporter

Comment 34

•

8 years ago

I also ran a Try push on the commit immediately prior to the tests being moved and they were already not crashing (they were even more leaky, though). I can try to bisect the specific fix if you really want to know, but I believe that the basic answer is no, moving the tests didn't wallpaper the problem.

Comment 35

•

8 years ago

(In reply to :Felipe Gomes (needinfo me!) from comment #33)
> I'd also be really interested to know if the crash is gone by moving back
> the tests where they were, but using current m-c

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> I also ran a Try push on the commit immediately prior to the tests being
> moved and they were already not crashing (they were even more leaky,
> though). I can try to bisect the specific fix if you really want to know,
> but I believe that the basic answer is no, moving the tests didn't wallpaper
> the problem.

I reverted bug 1265055 (hope this is enough) and still no crash:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d374bfe790e4

Flags: needinfo?(wpan)

Andrew McCreight [:mccr8]

Reporter

Comment 36

•

8 years ago

Would be nice if we could figure out what's causing the Win8 leaks.

Comment 37

•

8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> Would be nice if we could figure out what's causing the Win8 leaks.

Please file a separate bug for that, in Widget or Drag and Drop.