Closed Bug 676828 Opened 9 years ago Closed 5 years ago

ABORT: State invariants violated: 'mState == FAILED || mState == STARTED || mState == CLONED', file c:/mozilla/mozilla-central/widget/src/windows/AudioSession.cpp

Categories

(Core :: Widget, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mak, Assigned: m_kato)

References

Details

(Keywords: testcase)

Attachments

(4 files)

while running xpcshell tests I often get this abort

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/AudioSession.cpp#373

this is the stack

>	xul.dll!mozilla::widget::AudioSession::GetSessionData(nsID & aID, nsString & aSessionName, nsString & aIconPath)  Line 373	C++
 	xul.dll!mozilla::widget::GetAudioSessionData(nsID & aID, nsString & aSessionName, nsString & aIconPath)  Line 144	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::NP_Initialize(_NPNetscapeFuncs * bFuncs, short * error)  Line 801 + 0x11 bytes	C++
 	xul.dll!nsNPAPIPlugin::CreatePlugin(nsPluginTag * aPluginTag, nsNPAPIPlugin * * aResult)  Line 499 + 0x16 bytes	C++
 	xul.dll!CreateNPAPIPlugin(nsPluginTag * aPluginTag, nsNPAPIPlugin * * aOutNPAPIPlugin)  Line 1641 + 0xd bytes	C++
 	xul.dll!nsPluginHost::EnsurePluginLoaded(nsPluginTag * plugin)  Line 1650 + 0x21 bytes	C++
 	xul.dll!nsPluginHost::ClearSiteData(nsIPluginTag * plugin, const nsACString_internal & domain, unsigned __int64 flags, __int64 maxAge)  Line 1816 + 0xc bytes	C++

looking at the code (and in the debugger) we invoke GetAudioSessionData before StartAudioSession()
So, since this seems to be the parent process, the call should be the one in nsAppShell? That doesn't seem to run for xpcshell tests
I don't see this locally.  Can you give STR?
somehow I missed bugmail along august...

So I don't have good STR I just run xpcshell tests in browser/components/places/tests/ and one of these tests simulate a shutdown cleanup. that test on my Win7 machine constantly hits the above abort.
Ok, I see this locally.  I'll poke at it when I have some time.
the test is test_clearHistory_shutdown.js
the test is simulating a sanitize() on shutdown, that may even try to init plugins by clearing cookies or similar unexpected stuff... Making the same test as a b-c test would probably solve the problem, but that's not feasible due to the fact b-c tests don't create new instances for each test, making hard to simulate shutdown tasks.
indeed I can confirm the issue is due to trying the removal of cookies, that includes plug-ins data.
Can we spin up the appshell somehow?

If not, idk what we can do here ...
I can easily fix the test (just flip cookies removal from true to false, they are not needed for this actual test), I just wonder if we should handle the cases where GetAudioSessionData may be invoked before StartAudioSession. I you think doesn't matter I'll just fix the test.
Hitting this when fuzzing Firefox on Windows, too. It seems to happen on shutdown, when Firefox has restarted after an abort.
This bug is preventing me from investigating some <audio>-related crashes.
I've been using a locally-built 64-bit debug version of Firefox for my everyday browsing and I see this abort very frequently; in fact I think this is happening every time I try to load Flash content. mState is always 0 (UNINITIALIZED).

I wasn't able to break in mozilla::widget::StartAudioSession() so I'm wondering whether that ever gets called.  MXR shows only two locations where we call mozilla::widget::StartAudioSession() and one of them is #ifdef'd out on x64 [https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp?rev=7326d36d774e#265].
Attached file testcase.swf
actually contains text.
reproduced on Beta/14, Aurora/15 and Nightly/16 with Flash 11.3.300.262 on Windows 7 64bit but not Windows 7 32bit.
Keywords: testcase
Jim Mathies recently fixed an issue where `StartAudioSession()` was not being called on Windows 64 builds - see bug 780333.
Depends on: 780333
When test environment has Flash, it still occurs due to another reason.  Since Flash uses async init, AudioSession has to be intialized.

AudioSession is initialized on nsAppShell::Run, but xpcshell won't call it.
Assignee: nobody → m_kato
Comment on attachment 8707300 [details] [diff] [review]
Initialize AudioSession on xpcshell

browser/components/places/tests/unit/test_clearHistory_shutdown.js hits assertion on my test environment.

This test will call nsIPluginHost.clearSiteData.  If Flash is installed, it can be initialized asynchronized.  When this situation, RecvNP_InitializeResult is called, then it tries to access AudioSession.  Since AudiSession isn't initialized on xpcshell, this assertion occurs.

So we should call StartAduioSession on xpcshell startup.
Attachment #8707300 - Flags: review?(jmathies)
Attachment #8707300 - Flags: review?(jmathies) → review+
I don't see how this fixes comment 11, but maybe that's not valid anymore. I've not seen any bug reports on it.
Blocks: 1201904
https://hg.mozilla.org/mozilla-central/rev/3c18be4736c7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Nit: This probably should have gotten review review from an XPConnect peer, who would have probably delegated to glandium or ted.

glandium, does this seem ok to you?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8707300 [details] [diff] [review]
Initialize AudioSession on xpcshell

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

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1569,5 @@
>              JS_GC(rt);
>          }
>          JS_GC(rt);
> +#ifdef XP_WIN
> +        widget::StopAudioSession();

There are plenty of code paths that leave the function without calling StopAudioSession... a RAII wrapper may have been better?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #22)

> There are plenty of code paths that leave the function without calling
> StopAudioSession... a RAII wrapper may have been better?

Ok - but no weird linkage or automation issues from spinning up audio stuff here? I guess we also spin up graphics.

NI Makoto to fix things up to use an RAII guard.
Flags: needinfo?(m_kato)
(In reply to Bobby Holley (busy) from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #22)
> 
> > There are plenty of code paths that leave the function without calling
> > StopAudioSession... a RAII wrapper may have been better?
> 
> Ok - but no weird linkage or automation issues from spinning up audio stuff
> here? I guess we also spin up graphics.

I don't expect anything weird there. That being said, if widget code expects audiosession to be initialized, why isn't the initialization done in widget?
Flags: needinfo?(jmathies)
Attached patch Use RIIASplinter Review
Flags: needinfo?(m_kato)
Comment on attachment 8710307 [details] [diff] [review]
Use RIIA

Use RAII instead
Attachment #8710307 - Flags: review?(bobbyholley)
Comment on attachment 8710307 [details] [diff] [review]
Use RIIA

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

r=me with that.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +98,5 @@
>      nsCOMPtr<nsIFile> mAppFile;
>  };
>  
> +#ifdef XP_WIN
> +class AutoAudioSession

Please annotate this with MOZ_STACK_CLASS.
Attachment #8710307 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (busy) from comment #21)
> Nit: This probably should have gotten review review from an XPConnect peer,
> who would have probably delegated to glandium or ted.
> 
> glandium, does this seem ok to you?

Sorry should have thought of that. The whole peers thing is a bit blurry these days.

(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Bobby Holley (busy) from comment #23)
> > (In reply to Mike Hommey [:glandium] from comment #22)
> > 
> > > There are plenty of code paths that leave the function without calling
> > > StopAudioSession... a RAII wrapper may have been better?
> > 
> > Ok - but no weird linkage or automation issues from spinning up audio stuff
> > here? I guess we also spin up graphics.
> 
> I don't expect anything weird there. That being said, if widget code expects
> audiosession to be initialized, why isn't the initialization done in widget?

It is, but it doesn't get hit in xpcshell.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#251
Flags: needinfo?(jmathies)
You need to log in before you can comment on or make changes to this bug.