Closed Bug 1183651 Opened 9 years ago Closed 9 years ago

Intermittent test_fullscreen-api.html,test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()] or [@ NS_CycleCollectorSuspect3][@ nsGlobalWindow::Release()]

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: RyanVM, Assigned: xidorn)

References

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file)

patch
1.48 KB, patch
jimm
: review+
Details | Diff | Splinter Review 
19:33:46 INFO - 937 INFO TEST-START | dom/html/test/test_fullscreen-api-race.html
19:33:46 INFO - ++DOMWINDOW == 59 (10043400) [pid = 3768] [serial = 1814] [outer = 1ABB7C00]
19:33:46 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
19:33:46 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
19:33:46 INFO - nsBrowserAccess.prototype.openURI@chrome://browser/content/browser.js:15965:21
19:33:46 INFO - openNewTab@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:27:12
19:33:46 INFO - next@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:110:13
19:33:46 INFO - ++DOCSHELL 10E3B000 == 11 [pid = 3768] [id = 300]
19:33:46 INFO - ++DOMWINDOW == 60 (13C65000) [pid = 3768] [serial = 1815] [outer = 00000000]
19:33:46 INFO - ++DOMWINDOW == 61 (141D7400) [pid = 3768] [serial = 1816] [outer = 13C65000]
19:33:46 INFO - ++DOMWINDOW == 62 (141DB000) [pid = 3768] [serial = 1817] [outer = 13C65000]
19:33:47 INFO - ++DOMWINDOW == 63 (14681400) [pid = 3768] [serial = 1818] [outer = 13C65000]
19:33:47 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
19:33:47 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
19:33:47 INFO - nsBrowserAccess.prototype.openURI@chrome://browser/content/browser.js:15965:21
19:33:47 INFO - openNewTab@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:27:12
19:33:47 INFO - next@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:110:13
19:33:47 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:736:59
19:33:47 INFO - ++DOCSHELL 146EC000 == 12 [pid = 3768] [id = 301]
19:33:47 INFO - ++DOMWINDOW == 64 (146ED800) [pid = 3768] [serial = 1819] [outer = 00000000]
19:33:47 INFO - ++DOMWINDOW == 65 (149BF800) [pid = 3768] [serial = 1820] [outer = 146ED800]
19:33:47 INFO - ++DOMWINDOW == 66 (1568F000) [pid = 3768] [serial = 1821] [outer = 146ED800]
19:33:47 INFO - [Parent 3768] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file c:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/src/dom/base/nsGlobalWindow.cpp, line 6363
19:33:48 INFO - [Parent 3768] WARNING: Fix the caller!: file c:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/src/dom/events/EventDispatcher.cpp, line 487
19:33:48 INFO - [Parent 3768] WARNING: NS_ENSURE_TRUE(mCallback) failed: file c:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/src/dom/base/nsFrameMessageManager.cpp, line 803
19:33:48 INFO - WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
19:33:48 INFO - pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
19:33:48 INFO - nsBrowserAccess.prototype.openURI@chrome://browser/content/browser.js:15965:21
19:33:48 INFO - openNewTab@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:27:12
19:33:48 INFO - next@http://mochi.test:8888/tests/dom/html/test/test_fullscreen-api-race.html:110:13
19:33:48 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:736:59
19:33:48 INFO - ++DOCSHELL 15A3A400 == 13 [pid = 3768] [id = 302]
19:33:48 INFO - ++DOMWINDOW == 67 (15A3AC00) [pid = 3768] [serial = 1822] [outer = 00000000]
19:33:48 INFO - ++DOMWINDOW == 68 (15A7DC00) [pid = 3768] [serial = 1823] [outer = 15A3AC00]
19:33:48 INFO - ++DOMWINDOW == 69 (15A83800) [pid = 3768] [serial = 1824] [outer = 15A3AC00]
19:33:48 INFO - [Parent 3768] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file c:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/src/dom/base/nsGlobalWindow.cpp, line 6363
19:33:49 INFO - Hit MOZ_CRASH(FullscreenTransitionData not thread-safe) at c:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/src/widget/windows/nsWindow.cpp:2985
19:34:03 INFO - #01: nsCOMPtr_base::~nsCOMPtr_base() [xpcom/glue/nsCOMPtr.h:296]
19:34:03 INFO - #02: nsRunnable::Release() [xpcom/glue/nsThreadUtils.cpp:32]
19:34:03 INFO - #03: nsCOMPtr<nsIRunnable>::~nsCOMPtr<nsIRunnable>() [xpcom/glue/nsCOMPtr.h:391]
19:34:03 INFO - #04: USER32 + 0x1c4e7
19:34:03 INFO - #05: USER32 + 0x1c5e7
19:34:03 INFO - #06: USER32 + 0x1cc19
19:34:03 INFO - #07: USER32 + 0x1cc70
19:34:03 INFO - #08: FullscreenTransitionThreadProc [widget/windows/nsWindow.cpp:2953]
19:34:03 INFO - #09: kernel32 + 0x53c45
19:34:03 INFO - #10: ntdll + 0x637f5
19:34:03 INFO - #11: ntdll + 0x637c8
19:34:03 INFO - TEST-INFO | Main app process: exit status 1
19:34:03 INFO - 938 INFO Testing openNewTab, navigate
19:34:03 INFO - 939 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | Should have entered fullscreen
19:34:03 INFO - 940 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should be in fullscreen
19:34:03 INFO - 941 INFO About to navigate to another page
19:34:03 INFO - 942 INFO must wait for load
19:34:03 INFO - 943 INFO must wait for focus
19:34:03 INFO - 944 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should no longer be in fullscreen
19:34:03 INFO - 945 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should have been closed
19:34:03 INFO - 946 INFO Testing openNewTab, closeWindow
19:34:03 INFO - 947 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | Should have entered fullscreen
19:34:03 INFO - 948 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should be in fullscreen
19:34:03 INFO - 949 INFO About to close the window
19:34:03 INFO - 950 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should have been closed
19:34:03 INFO - 951 INFO Testing openNewTab, exitFullscreen
19:34:03 INFO - 952 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | Should have entered fullscreen
19:34:03 INFO - 953 INFO TEST-PASS | dom/html/test/test_fullscreen-api-race.html | The window should be in fullscreen
19:34:03 INFO - 954 INFO About to cancel fullscreen
19:34:03 WARNING - TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api-race.html | application terminated with exit code 1
19:34:03 INFO - runtests.py | Application ran for: 0:07:35.518000
19:34:03 INFO - zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpgu3vthpidlog
19:34:14 INFO - mozcrash Saved minidump as C:\slave\test\build\blobber_upload_dir\a043d448-67db-43d3-b458-bf751a3c4d78.dmp
19:34:14 INFO - mozcrash Saved app info as C:\slave\test\build\blobber_upload_dir\a043d448-67db-43d3-b458-bf751a3c4d78.extra
19:34:14 WARNING - PROCESS-CRASH | dom/html/test/test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()]
19:34:14 INFO - Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpokroae.mozrunner\minidumps\a043d448-67db-43d3-b458-bf751a3c4d78.dmp
19:34:14 INFO - Operating system: Windows NT
19:34:14 INFO - 6.1.7601 Service Pack 1
19:34:14 INFO - CPU: x86
19:34:14 INFO - GenuineIntel family 6 model 30 stepping 5
19:34:14 INFO - 8 CPUs
19:34:14 INFO - Crash reason: EXCEPTION_BREAKPOINT
19:34:14 INFO - Crash address: 0x6541e32b
19:34:14 INFO - Thread 66 (crashed)
19:34:14 INFO - 0 xul.dll!FullscreenTransitionData::Release() [nsWindow.cpp:4f939aa499b0 : 2985 + 0x26]
19:34:14 INFO - eip = 0x6541e32b esp = 0x18f4fc90 ebp = 0x18f4fc98 ebx = 0x00000000
19:34:14 INFO - esi = 0x00000ba9 edi = 0x16c490f0 eax = 0x00000000 ecx = 0x699f0ad9
19:34:14 INFO - edx = 0x0030bdb9 efl = 0x00000216
19:34:14 INFO - Found by: given as instruction pointer in context
19:34:14 INFO - 1 xul.dll!nsCOMPtr_base::~nsCOMPtr_base() [nsCOMPtr.h:4f939aa499b0 : 296 + 0x5]
19:34:14 INFO - eip = 0x63ab9a99 esp = 0x18f4fca0 ebp = 0x18f4fcb8
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 2 xul.dll!FullscreenTransitionTask::~FullscreenTransitionTask() + 0xa
19:34:14 INFO - eip = 0x64528b94 esp = 0x18f4fcac ebp = 0x18f4fcb8
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 3 xul.dll!FullscreenTransitionTask::`scalar deleting destructor'(unsigned int) + 0xa
19:34:14 INFO - eip = 0x6452aa7f esp = 0x18f4fcb4 ebp = 0x18f4fcb8
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 4 xul.dll!nsRunnable::Release() [nsThreadUtils.cpp:4f939aa499b0 : 32 + 0x6c]
19:34:14 INFO - eip = 0x63b45b91 esp = 0x18f4fcc0 ebp = 0x18f4fcd0
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 5 xul.dll!nsCOMPtr<nsIRunnable>::~nsCOMPtr<nsIRunnable>() [nsCOMPtr.h:4f939aa499b0 : 391 + 0x5]
19:34:14 INFO - eip = 0x63ab9336 esp = 0x18f4fcd8 ebp = 0x18f4fce8
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 6 xul.dll!FullscreenTransitionWindowProc [nsWindow.cpp:4f939aa499b0 : 2879 + 0x7]
19:34:14 INFO - eip = 0x65417804 esp = 0x18f4fce4 ebp = 0x18f4fce8
19:34:14 INFO - Found by: call frame info
19:34:14 INFO - 7 user32.dll + 0x1c4e6
19:34:14 INFO - eip = 0x75ffc4e7 esp = 0x18f4fcf0 ebp = 0x18f4fd14
19:34:14 INFO - Found by: call frame info
Flags: needinfo?(quanxunzhen)
Ahh... A race condition there...
Summary: Intermittent test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()] → Intermittent test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()] or [@ NS_CycleCollectorSuspect3][@ nsGlobalWindow::Release()]
Summary: Intermittent test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()] or [@ NS_CycleCollectorSuspect3][@ nsGlobalWindow::Release()] → Intermittent test_fullscreen-api.html,test_fullscreen-api-race.html | application crashed [@ FullscreenTransitionData::Release()] or [@ NS_CycleCollectorSuspect3][@ nsGlobalWindow::Release()]
Depends on: 1155059
Attached patch patch — — Splinter Review 
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)

        Attachment #8633793 -
        Flags: review?(jmathies)

    
    

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

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

::: widget/windows/nsWindow.cpp
@@ -2874,5 @@
>        if (uMsg == WM_FULLSCREEN_TRANSITION_AFTER) {
>          flags |= AW_HIDE;
>        }
>        ::AnimateWindow(hWnd, duration, flags);
> -      NS_DispatchToMainThread(callback);

This usage (a local nsCOMPtr, and not using .forget() with DispatchToMainThread (or Dispatch)) means there's no guarantee which thread the runnable will be deleted on (I presume that's what causes this bug).

Moving towards eliminating the non-already_AddRefed version is the point of the patch I just landed; I'm working on patches to convert of the tree, one directory-group at a time.

    
    

    
  
Just to note: it should be reliably released on Mainthread with this patch.

    
    

    
  
(In reply to Randell Jesup [:jesup] from comment #11)
> Moving towards eliminating the non-already_AddRefed version is the point of
> the patch I just landed; I'm working on patches to convert of the tree, one
> directory-group at a time.

I'd suggest we keep accepting the raw pointer so that we don't need to add extra code for the common usage which passes in a newly created runnable. To avoid those functions being used with nsCOMPtr, we probably can use a template overload with static_assert to make it a sound compile error.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #13)
> (In reply to Randell Jesup [:jesup] from comment #11)
> > Moving towards eliminating the non-already_AddRefed version is the point of
> > the patch I just landed; I'm working on patches to convert of the tree, one
> > directory-group at a time.
> 
> I'd suggest we keep accepting the raw pointer so that we don't need to add
> extra code for the common usage which passes in a newly created runnable. To
> avoid those functions being used with nsCOMPtr, we probably can use a
> template overload with static_assert to make it a sound compile error.

Can we make it do such a static assert reliably?  I'm preparing the first batch of patches to convert things, so knowing if this is possible to do (or even in a static-analysis build) would greatly simplify the patches.
Note that in any case we need to AddRef() once; the issue is if it's done by the caller or the callee in this pretty common case.

Old:
  Dispatch(new FooRunnable(...), NS_DISPATCH_NORMAL)

New (currently):
  Dispatch(do_AddRef(new FooRunnable(...)), NS_DISPATCH_NORMAL)

(We added do_AddRef() as part of the Dispatch changes.)

I'm fine with allowing Dispatch(new ...) if we can block other uses.  Also note there are a TON of:
  nsRefPtr<FooRunnable> foo = new FooRunnable(...);
  DispatchToMainThread(foo);
which will still need conversion to avoid release-races (and the extra AddRef/Release), but we can convert to:
  DispatchToMainThread(new FooRunnable(...));
instead of using
  DispatchToMainThread(do_AddRef(new FooRunnable(...)));
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(nfroyd)

    
    

    
  
(In reply to Randell Jesup [:jesup] from comment #14)
> Can we make it do such a static assert reliably?  I'm preparing the first
> batch of patches to convert things, so knowing if this is possible to do (or
> even in a static-analysis build) would greatly simplify the patches.

I think we can. But yes, we need to ensure it disables every refcount pointer.

Try code like this: https://pastebin.mozilla.org/8839564
Flags: needinfo?(quanxunzhen)

    
  

        Attachment #8633793 -
        Flags: review?(jmathies) → review+

    
    

    
  
I think the best way is something like this:

> template<class T, typename = typename mozilla::EnableIf<
>   !mozilla::IsPointer<T>::value &&
>   mozilla::IsConvertible<T, nsIRunnable*>::value>::type>
> NS_METHOD
> NS_DispatchToMainThread(T, uint32_t = NS_DISPATCH_NORMAL) = delete;

For calling with any non-pointer type which is implicitly convertible to nsIRunnable*, it gives errors like:
> use of deleted function 'NS_METHOD NS_DispatchToMainThread(T, uint32_t) [with T = nsCOMPtr<nsIRunnable>; <template-parameter-1-2> = void]'

For calling with any unrelated type, it gives errors like:
> no matching function for call to 'NS_DispatchToMainThread(nsCOMPtr<int>&)'
Flags: needinfo?(rjesup)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #16)
> I think the best way is something like this:
> 
> > template<class T, typename = typename mozilla::EnableIf<
> >   !mozilla::IsPointer<T>::value &&
> >   mozilla::IsConvertible<T, nsIRunnable*>::value>::type>
> > NS_METHOD
> > NS_DispatchToMainThread(T, uint32_t = NS_DISPATCH_NORMAL) = delete;
> 
> For calling with any non-pointer type which is implicitly convertible to
> nsIRunnable*, it gives errors like:
> > use of deleted function 'NS_METHOD NS_DispatchToMainThread(T, uint32_t) [with T = nsCOMPtr<nsIRunnable>; <template-parameter-1-2> = void]'
> 
> For calling with any unrelated type, it gives errors like:
> > no matching function for call to 'NS_DispatchToMainThread(nsCOMPtr<int>&)'

I'm afraid on GCC that doesn't trap any uses of DispatchToMainThread(smart_ptr)

The previous version works:

template<class T>
struct TypeAllowed
{
  static const bool value = false;
};

template<class T>
NS_METHOD
NS_DispatchToMainThread(T, uint32_t = 0)
{
  static_assert(TypeAllowed<T>::value, "don't use NS_DispatchToMainThread with nsCOMPtr/nsRefPtr, "
                "call pointer.forget() instead");
  return NS_OK;
}
Flags: needinfo?(rjesup)

    
    

    
      
  

    
    

    
  
(In reply to Randell Jesup [:jesup] from comment #17)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #16)
> > I think the best way is something like this:
> > 
> > > template<class T, typename = typename mozilla::EnableIf<
> > >   !mozilla::IsPointer<T>::value &&
> > >   mozilla::IsConvertible<T, nsIRunnable*>::value>::type>
> > > NS_METHOD
> > > NS_DispatchToMainThread(T, uint32_t = NS_DISPATCH_NORMAL) = delete;
> > 
> > For calling with any non-pointer type which is implicitly convertible to
> > nsIRunnable*, it gives errors like:
> > > use of deleted function 'NS_METHOD NS_DispatchToMainThread(T, uint32_t) [with T = nsCOMPtr<nsIRunnable>; <template-parameter-1-2> = void]'
> > 
> > For calling with any unrelated type, it gives errors like:
> > > no matching function for call to 'NS_DispatchToMainThread(nsCOMPtr<int>&)'
> 
> I'm afraid on GCC that doesn't trap any uses of
> DispatchToMainThread(smart_ptr)

I tested on GCC and it traps... with the stdlib equivalent. If it doesn't trap with mfbt impl, there is probably some problem with our impl. I'll test.

> The previous version works:
> 
> template<class T>
> struct TypeAllowed
> {
>   static const bool value = false;
> };
> 
> template<class T>
> NS_METHOD
> NS_DispatchToMainThread(T, uint32_t = 0)
> {
>   static_assert(TypeAllowed<T>::value, "don't use NS_DispatchToMainThread
> with nsCOMPtr/nsRefPtr, "
>                 "call pointer.forget() instead");
>   return NS_OK;
> }

But I really don't like this.

Should we have a bug somewhere about converting the existing code and adding this guard?

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/79578dac895f
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox42:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1151643
Crash Signature: [@ NS_CycleCollectorSuspect3]

    
    

    
  
(I added the crash signature because this issue is showing up on Nightly as well.)
Flags: needinfo?(nfroyd)
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: