1144750 - test-selection.js asserts and crashes in windows debug builds

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Dave Townsend [:mossop]]

17:26:56 INFO - TEST-START | jetpack-package/addon-sdk/source/test/test-selection.js.test Selection Listener on frame
17:26:57 INFO - ++DOCSHELL 1A361000 == 33 [pid = 2820] [id = 357]
17:26:57 INFO - ++DOMWINDOW == 140 (1A86C080) [pid = 2820] [serial = 946] [outer = 00000000]
17:26:57 INFO - ++DOMWINDOW == 141 (1A86CA80) [pid = 2820] [serial = 947] [outer = 1A86C080]
17:26:57 INFO - [2820] WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\netwerk\base\nsSimpleURI.cpp, line 265
17:26:57 INFO - --DOMWINDOW == 140 (10653200) [pid = 2820] [serial = 893] [outer = 00000000] [url = data:text/html;charset=utf-8,%3Chtml%3E%20%20%3Cbody%3E%20%20%20%20%3Cdiv%3Efoo%3C%2Fdiv%3E%20%20%20%20%3Cdiv%3Eand%3C%2Fdiv%3E%20%20%20%20%3Ctextarea%3Enoodles%3C%2Ftextarea%3E%20%20%3C%2Fbody%3E%3C%2Fhtml%3E]
17:26:57 INFO - --DOMWINDOW == 139 (0F4B4280) [pid = 2820] [serial = 887] [outer = 00000000] [url = data:text/html;charset=utf-8,%3Chtml%3E%20%20%3Cbody%3E%20%20%20%20%3Cdiv%3Efoo%3C%2Fdiv%3E%20%20%20%20%3Cdiv%3Eand%3C%2Fdiv%3E%20%20%20%20%3Ctextarea%3Enoodles%3C%2Ftextarea%3E%20%20%3C%2Fbody%3E%3C%2Fhtml%3E]
17:26:57 INFO - --DOMWINDOW == 138 (10650A00) [pid = 2820] [serial = 889] [outer = 00000000] [url = data:text/html;charset=utf-8,%3Chtml%3E%20%20%3Cbody%3E%20%20%20%20%3Cdiv%3Efoo%3C%2Fdiv%3E%20%20%20%20%3Cdiv%3Eand%3C%2Fdiv%3E%20%20%20%20%3Ctextarea%3Enoodles%3C%2Ftextarea%3E%20%20%3C%2Fbody%3E%3C%2Fhtml%3E]
17:26:57 INFO - --DOMWINDOW == 137 (10652A80) [pid = 2820] [serial = 891] [outer = 00000000] [url = data:text/html;charset=utf-8,%3Chtml%3E%20%20%3Cbody%3E%20%20%20%20%3Cdiv%3Efoo%3C%2Fdiv%3E%20%20%20%20%3Cdiv%3Eand%3C%2Fdiv%3E%20%20%20%20%3Ctextarea%3Enoodles%3C%2Ftextarea%3E%20%20%3C%2Fbody%3E%3C%2Fhtml%3E]
17:26:57 INFO - ++DOCSHELL 1ADFB500 == 34 [pid = 2820] [id = 358]
17:26:57 INFO - ++DOMWINDOW == 138 (10650A00) [pid = 2820] [serial = 948] [outer = 00000000]
17:26:57 INFO - ++DOMWINDOW == 139 (1115FE00) [pid = 2820] [serial = 949] [outer = 10650A00]
17:26:57 INFO - Assertion failure: (((aRep->flags) & 0x1) != 0), at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\dom\base\ScriptSettings.cpp:484
17:26:57 INFO - #01: js::CallErrorReporter(JSContext *,char const *,JSErrorReport *) [js/src/jscntxt.cpp:794]
17:26:57 INFO - #02: ReportError [js/src/jscntxt.cpp:238]
17:26:57 INFO - #03: js::ReportErrorNumberVA(JSContext *,unsigned int,JSErrorFormatString const * (*)(void *,unsigned int),void *,unsigned int,js::ErrorArgumentsType,char *) [js/src/jscntxt.cpp:739]
17:26:57 INFO - #04: JS_ReportErrorNumberUC(JSContext *,JSErrorFormatString const * (*)(void *,unsigned int),void *,unsigned int,...) [js/src/jsapi.cpp:5071]
17:26:57 INFO - #05: js::AutoEnterPolicy::reportErrorIfExceptionIsNotPending(JSContext *,jsid) [js/src/proxy/Proxy.cpp:52]
17:26:57 INFO - #06: js::AutoEnterPolicy::AutoEnterPolicy(JSContext *,js::BaseProxyHandler const *,JS::Handle<JSObject *>,JS::Handle<jsid>,unsigned int,bool) [js/public/Proxy.h:618]
17:26:57 INFO - #07: js::Proxy::get(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,JS::Handle<jsid>,JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:276]
17:26:57 INFO - #08: js::proxy_GetProperty(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,JS::Handle<jsid>,JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:585]
17:26:57 INFO - #09: MaybeCallMethod [js/src/jsobj.cpp:3430]
17:26:57 INFO - #10: JS::OrdinaryToPrimitive(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/src/jsobj.cpp:3459]
17:26:57 INFO - #11: js::SecurityWrapper<js::CrossCompartmentWrapper>::defaultValue(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/src/proxy/SecurityWrapper.cpp:79]
17:26:57 INFO - #12: xpc::FilteringWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>,xpc::Opaque>::defaultValue(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/xpconnect/wrappers/FilteringWrapper.cpp:163]
17:26:57 INFO - #13: js::Proxy::defaultValue(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:493]
17:26:57 INFO - #14: js::proxy_Convert(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:666]
17:26:57 INFO - #15: js::ToPrimitive(JSContext *,JS::Handle<JSObject *>,JSType,JS::MutableHandle<JS::Value>) [js/src/jsobj.cpp:3310]
17:26:57 INFO - #16: js::ToPrimitive(JSContext *,JSType,JS::MutableHandle<JS::Value>) [js/src/jsobjinlines.h:544]
17:26:57 INFO - #17: js::ToStringSlow<1>(js::ExclusiveContext *,JS::Handle<JS::Value>) [js/src/jsstr.cpp:4202]
17:26:57 INFO - #18: js::ErrorReport::init(JSContext *,JS::Handle<JS::Value>) [js/src/jsexn.cpp:744]
17:26:57 INFO - #19: mozilla::dom::AutoJSAPI::~AutoJSAPI() [dom/base/ScriptSettings.cpp:331]
17:26:57 INFO - #20: nsXBLProtoImplAnonymousMethod::Execute(nsIContent *,JSAddonId *) [dom/xbl/nsXBLProtoImplMethod.cpp:336]
17:26:57 INFO - #21: nsXBLPrototypeBinding::BindingAttached(nsIContent *) [dom/xbl/nsXBLPrototypeBinding.cpp:263]
17:26:57 INFO - #22: nsXBLBinding::ExecuteAttachedHandler() [dom/xbl/nsXBLBinding.cpp:610]
17:26:57 INFO - #23: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:4303]
17:26:57 INFO - #24: nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:1603]
17:26:57 INFO - #25: nsRefreshDriver::DoTick() [layout/base/nsRefreshDriver.cpp:1383]
17:26:57 INFO - #26: nsRefreshDriver::WillRefresh(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:1904]
17:26:57 INFO - #27: nsRefreshDriver::DoTick() [layout/base/nsRefreshDriver.cpp:1383]
17:26:57 INFO - #28: nsRefreshDriver::FinishedWaitingForTransaction() [layout/base/nsRefreshDriver.cpp:1844]
17:26:57 INFO - #29: mozilla::layers::ClientLayerManager::DidComposite(unsigned __int64) [gfx/layers/client/ClientLayerManager.cpp:395]
17:26:57 INFO - #30: mozilla::layers::CompositorChild::RecvDidComposite(unsigned __int64 const &,unsigned __int64 const &) [gfx/layers/ipc/CompositorChild.cpp:296]
17:26:57 INFO - #31: mozilla::layers::PCompositorChild::OnMessageReceived(IPC::Message const &) [obj-firefox/ipc/ipdl/PCompositorChild.cpp:820]
17:26:57 INFO - #32: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) [ipc/glue/MessageChannel.cpp:1218]
17:26:57 INFO - #33: mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const &) [ipc/glue/MessageChannel.cpp:1144]
17:26:57 INFO - #34: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() [ipc/glue/MessageChannel.cpp:1128]
17:26:57 INFO - #35: MessageLoop::RunTask(Task *) [ipc/chromium/src/base/message_loop.cc:362]
17:26:57 INFO - #36: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &) [ipc/chromium/src/base/message_loop.cc:372]
17:26:57 INFO - #37: MessageLoop::DoWork() [ipc/chromium/src/base/message_loop.cc:456]
17:26:57 INFO - #38: mozilla::ipc::DoWorkRunnable::Run() [ipc/glue/MessagePump.cpp:234]
17:26:57 INFO - #39: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:855]
17:26:57 INFO - #40: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/glue/nsThreadUtils.cpp:265]
17:26:57 INFO - #41: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:99]
17:26:57 INFO - #42: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:233]
17:26:57 INFO - #43: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:227]
17:26:57 INFO - #44: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:201]
17:26:57 INFO - #45: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:166]
17:26:57 INFO - #46: nsAppShell::Run() [widget/windows/nsAppShell.cpp:180]
17:26:57 INFO - #47: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:282]
17:26:57 INFO - #48: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4202]
17:26:57 INFO - #49: XREMain::XRE_main(int,char * * const,nsXREAppData const *) [toolkit/xre/nsAppRunner.cpp:4278]
17:26:57 INFO - #50: XRE_main [toolkit/xre/nsAppRunner.cpp:4498]
17:26:57 INFO - #51: do_main [browser/app/nsBrowserApp.cpp:294]
17:26:57 INFO - #52: NS_internal_main(int,char * *) [browser/app/nsBrowserApp.cpp:667]
17:26:57 INFO - #53: wmain [toolkit/xre/nsWindowsWMain.cpp:124]
17:26:57 INFO - #54: __tmainCRTStartup [f:/dd/vctools/crt/crtw32/startup/crt0.c:255]
17:26:57 INFO - #55: kernel32 + 0x17067

Comment 1

[Dave Townsend [:mossop]]

I can't reproduce this locally but looking at the stack this seems like the JS engine is doing something wrong? Bobby, you have blame for the assertion, can you shed any light here?

Comment 2

[Bobby Holley (:bholley)]

So. AutoJSAPI shims in WarningOnlyErrorReporter when it takes ownership of error reporting, because the JS engine is never supposed to call the error reporter for real errors when AutoJSPI is running the show.

But as the stack shows, we do indeed end up triggering the error reporter. This is because we end up recursively generating an error, and turning _that_ into an exception fails for some reason, and ReportError falls back to invoking the error reporter.

So I think we need ReportError (the one defined in jscntxt.cpp) to not fall back to the error reporter if the ErrorToException call fails. Ideally we'd NS_WARNING when that happens, but I don't think we can do that from within the JS engine. :-(

Should be a 1-line change (plus comment). I'm happy to r+ it if it's green on try.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch

I may be misunderstanding you. Here is what I tried but it came out very not green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66af64839c9d&exclusion_profile=false

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8580151 [details] [diff] [review]
patch

Review of attachment 8580151 [details] [diff] [review]:
-----------------------------------------------------------------

Not quite - I was suggesting making return following ErrorToException(...) unconditional, rather than only doing it in the case where ErrorToException succeeds.

Comment 5

[Dave Townsend [:mossop]]

So I tried returning unconditionally (https://hg.mozilla.org/try/rev/42906d06fe10) but that still caused a bunch of try failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f5221f449c. Then I tried only returning if it seems that AutoJSAPI owns error reporting (https://hg.mozilla.org/try/rev/e94179fc5756). That passes try, but didn't fix the failure :(

Comment 6

[Bobby Holley (:bholley)]

(In reply to Dave Townsend [:mossop] from comment #5)
> So I tried returning unconditionally
> (https://hg.mozilla.org/try/rev/42906d06fe10) but that still caused a bunch
> of try failures:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f5221f449c. Then I
> tried only returning if it seems that AutoJSAPI owns error reporting
> (https://hg.mozilla.org/try/rev/e94179fc5756). That passes try, but didn't
> fix the failure :(

Hm, that last patch really should do it, I'd think. Can you link me to the crash stack a la comment 0?

I found https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94179fc5756&exclusion_profile=false , but there doesn't seem to be anything useful in the JP logs.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Dave Townsend [:mossop] from comment #5)
> > So I tried returning unconditionally
> > (https://hg.mozilla.org/try/rev/42906d06fe10) but that still caused a bunch
> > of try failures:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f5221f449c. Then I
> > tried only returning if it seems that AutoJSAPI owns error reporting
> > (https://hg.mozilla.org/try/rev/e94179fc5756). That passes try, but didn't
> > fix the failure :(
> 
> Hm, that last patch really should do it, I'd think. Can you link me to the
> crash stack a la comment 0?
> 
> I found
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e94179fc5756&exclusion_profile=false , but there
> doesn't seem to be anything useful in the JP logs.

There are some other fixes that have to land before it will show up, this try run shows it though: https://treeherder.mozilla.org/logviewer.html#?job_id=5815869&repo=try

Comment 8

[Bobby Holley (:bholley)]

Hm, the last few frames of that stack appear garbled, which is very unfortunate - I have no idea how we end up invoking CallErrorReporter on that patch given your patch.

Comment 9

[Dave Townsend [:mossop]]

Oh it isn't the patch there causing it, it has probably been happening for a while but mochitest-jetpack hasn't been running for very long and for as long as it has been running the debug builds have been failing for other reasons too.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Dave Townsend [:mossop] from comment #9)
> Oh it isn't the patch there causing it, it has probably been happening for a
> while but mochitest-jetpack hasn't been running for very long and for as
> long as it has been running the debug builds have been failing for other
> reasons too.

Right, but given the second patch you linked to in comment 5 (unconditionally returning if autoJSAPIOwnsErrorReporting()), it seems like we should never be hitting that assertion.

Comment 11

[Dave Townsend [:mossop]]

Looks like this has been going on for a while, this is probably the same as bug 847840. There are some more stacks in there

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch

Getting somewhere slowly. This fixes the assertion, but a new assertion pops up in its place:

JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)
--DOCSHELL 0x13087c000 == 12 [pid = 3008] [id = 13]
Assertion failure: !JS_IsExceptionPending(cx), at ./NodeBinding.cpp:146
#01: mozilla::dom::NodeBinding::get_parentNode(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitGetterCallArgs) (.NodeBinding.cpp:146, in XUL)
#02: mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2422, in XUL)
#03: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:235, in XUL)
#04: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:502, in XUL)
#05: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:558, in XUL)
#06: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:628, in XUL)
#07: CallGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>) (NativeObject.cpp:1614, in XUL)
#08: bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (NativeObject.cpp:1661, in XUL)
#09: bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (NativeObject.cpp:1878, in XUL)
#10: js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (NativeObject.cpp:1912, in XUL)
#11: js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (NativeObject.h:1434, in XUL)
#12: GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) (Interpreter.cpp:260, in XUL)
#13: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2431, in XUL)
#14: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:452, in XUL)
#15: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:521, in XUL)
#16: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:558, in XUL)
#17: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:4335, in XUL)
#18: JS::Call(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.h:3837, in XUL)
#19: nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) (nsXBLProtoImplMethod.cpp:333, in XUL)
#20: nsXBLPrototypeBinding::BindingAttached(nsIContent*) (nsXBLPrototypeBinding.cpp:263, in XUL)
#21: nsXBLBinding::ExecuteAttachedHandler() (nsXBLBinding.cpp:610, in XUL)
#22: nsBindingManager::ProcessAttachedQueue(unsigned int) (nsBindingManager.cpp:435, in XUL)
#23: PresShell::Initialize(int, int) (nsPresShell.cpp:1927, in XUL)
#24: nsDocumentViewer::InitPresentationStuff(bool) (nsDocumentViewer.cpp:686, in XUL)
#25: nsDocumentViewer::Show() (nsDocumentViewer.cpp:2074, in XUL)
#26: nsDocShell::SetVisibility(bool) (nsDocShell.cpp:6358, in XUL)
#27: non-virtual thunk to nsDocShell::SetVisibility(bool) (nsDocShell.cpp:6363, in XUL)
#28: nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*) (nsFrameLoader.cpp:754, in XUL)
#29: nsSubDocumentFrame::ShowViewer() (nsSubDocumentFrame.cpp:173, in XUL)
#30: AsyncFrameInit::Run() (nsSubDocumentFrame.cpp:84, in XUL)
#31: nsContentUtils::RemoveScriptBlocker() (nsContentUtils.cpp:5052, in XUL)
#32: nsAutoScriptBlocker::~nsAutoScriptBlocker() (nsContentUtils.h:2420, in XUL)
#33: nsAutoScriptBlocker::~nsAutoScriptBlocker() (nsContentUtils.h:2420, in XUL)
#34: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (nsPresShell.cpp:4281, in XUL)
#35: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1607, in XUL)
#36: mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long long, mozilla::TimeStamp) (nsRefreshDriver.cpp:199, in XUL)
#37: mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp) (nsRefreshDriver.cpp:183, in XUL)
#38: mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) (nsRefreshDriver.cpp:441, in XUL)
#39: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) (nsRefreshDriver.cpp:376, in XUL)
#40: void nsRunnableMethodArguments<mozilla::TimeStamp>::apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)>(mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver*, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)) (nsThreadUtils.h:588, in XUL)
#41: nsRunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, mozilla::TimeStamp>::Run() (nsThreadUtils.h:666, in XUL)
#42: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:856, in XUL)
#43: NS_ProcessPendingEvents(nsIThread*, unsigned int) (nsThreadUtils.cpp:207, in XUL)
#44: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:99, in XUL)
#45: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:377, in XUL)
#46: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 17
#47: __CFRunLoopDoSources0 (in CoreFoundation) + 269
#48: __CFRunLoopRun (in CoreFoundation) + 927
#49: CFRunLoopRunSpecific (in CoreFoundation) + 296
#50: RunCurrentEventLoopInMode (in HIToolbox) + 235
#51: ReceiveNextEventCommon (in HIToolbox) + 431
#52: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
#53: _DPSNextEvent (in AppKit) + 964
#54: -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 194
#55: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (nsAppShell.mm:118, in XUL)
#56: -[NSApplication run] (in AppKit) + 594
#57: nsAppShell::Run() (nsAppShell.mm:651, in XUL)
#58: nsAppStartup::Run() (nsAppStartup.cpp:281, in XUL)
#59: XREMain::XRE_mainRun() (nsAppRunner.cpp:4202, in XUL)
#60: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:4278, in XUL)
#61: XRE_main (nsAppRunner.cpp:4498, in XUL)
#62: do_main(int, char**, nsIFile*) (nsBrowserApp.cpp:294, in firefox)
#63: main (nsBrowserApp.cpp:667, in firefox)

The bottom of the stack seems to match the last one but the assertion stems from an earlier point in nsXBLProtoImplAnonymousMethod::Execute which is weird but I guess it's just a later invocation or something. Still, that line looks a little suspect based on the comment above:

http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLProtoImplMethod.cpp#331

I did confirm that when the assertion happens AutoJSAPIOwnsErrorReporting is still true but I'm mostly stumbling in the dark here, any suggestions?

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8582509 [details] [diff] [review]
patch

Review of attachment 8582509 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ScriptSettings.cpp
@@ +343,5 @@
>      }
> +
> +    // Now that any pending exception has been reported reset
> +    // AutoJSAPIOwnsErrorReporting. This must be the last thing before resetting
> +    // the error reporter.

Explain why (i.e. mention the code in jscntxt.cpp).

Comment 14

[Bobby Holley (:bholley)]

So that assertion means that somebody is leaving an exception dangling on cx. My guess is that this is happening in ErrorReport::init (you'll see that it asserts against a pending exception at the top of the function). There should be no pending exception at the end of the AutoJSAPI destructor in the mOwnErrorReporting (i.e. !JS_IsExceptionPending), and there should be no pending exception when we return from ErrorReport::init.

These bugs are annoying but pretty straightforward to debug. Just add assertions against pending exceptions in places where there shouldn't be one, and work backwards until you find the culprit.

Culprits are usually someone that ignores failure return values from JSAPI methods. But the code in comment 12 looks ok, because the AutoEntryScript should be handling the exception as the comment suggests.

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch

Tracked it down. IsDuckTypedErrorObject can generate exceptions so I've made it clear them after each JSAPI call which seems to match what ErrorReport::init does too. Only the first actually matters but I figured it was a good idea to clear them throughout.

Comment 16

[Dave Townsend [:mossop]]

Oh and this has had a nice green try run aside from known failures

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8583880 [details] [diff] [review]
patch

Review of attachment 8583880 [details] [diff] [review]:
-----------------------------------------------------------------

Is there any reason to add all the exception clearing in IsDuckTypedErrorObject rather than just doing it in the caller?

::: dom/base/ScriptSettings.cpp
@@ +343,5 @@
>      }
> +
> +    // Now that any pending exception has been reported reset
> +    // AutoJSAPIOwnsErrorReporting. This must be the last thing before resetting
> +    // the error reporter or ReportError may try to send errors to our error

This is still a tiny bit too vague. I'd say:

We need to do this _after_ processing the existing exception, because the JS engine can throw while doing that, and uses this bit to determine what to do in that case: squelch the exception if the bit is set, otherwise call the error reporter. Calling WarningOnlyErrorReporter with a non-warning will assert, so we need to make sure we do the former.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Bobby Holley (:bholley) from comment #17)
> Comment on attachment 8583880 [details] [diff] [review]
> patch
> 
> Review of attachment 8583880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there any reason to add all the exception clearing in
> IsDuckTypedErrorObject rather than just doing it in the caller?

I don't think it makes much difference but since it's called in an if statement It would be simplest to just clear the exception at the end of this large if block: http://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp#760 at which point I might as well remove all the exception clearing that goes on in there. Either that or define a new boolean with the result that the if statement uses.

Not sure which you'd prefer.

Comment 19

[Bobby Holley (:bholley)]

Oh I see. How about just creating a tiny RAII class called AutoClearPendingException and declaring one in IsDuckTypedErrorObject? That's the cleanest thing I can think of anyway.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8584103 [details] [diff] [review]
patch

Review of attachment 8584103 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! thanks.

::: js/src/jsexn.h
@@ +119,5 @@
> +{
> +    JSContext *cx;
> +
> +  public:
> +    AutoClearPendingException(JSContext *cx)

nit - call this cxArg to disambiguate the member.

Comment 22

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/7588e6588dd3

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for static analysis failures: https://hg.mozilla.org/integration/fx-team/rev/08cef2ba28a0

https://treeherder.mozilla.org/logviewer.html#?job_id=2425859&repo=fx-team

Comment 24

[Bobby Holley (:bholley)]

Ctor needs an 'explicit' keyword.

Comment 25

[Dave Townsend [:mossop]]

Pushed with that fix: https://hg.mozilla.org/integration/fx-team/rev/29d740684613

Comment 26

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/29d740684613