These changes will make mozilla::LinkedList a bit safer. Patch in a moment.

This may strike one as inconsistent -- why silently do the right thing in one case and assert in the other? Part of the reason is that I don't want to do an O(n) operation (clearing a linked list) silently. But also, removing an element from the list when it's destroyed seems like what you'd want 90% of the time.

This patch would have let us avoid a security bug (bug 804041).

Comment on attachment 673406 [details] [diff] [review] Patch, v1 Review of attachment 673406 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable. ::: mfbt/LinkedList.h @@ +104,5 @@ > public: > + LinkedListElement() > + : next(this) > + , prev(this) > + , isSentinel(false) I changed this a little on Friday, so this hunk doesn't need to be changed, but mfbt/STYLE has commas at the end of the line. @@ +110,5 @@ > + > + ~LinkedListElement() { > + if (!isSentinel && isInList()) { > + remove(); > + } mfbt doesn't brace single-line ifs. @@ +248,5 @@ > > public: > + LinkedList() > + : sentinel(LinkedListElement<T>::NODE_KIND_SENTINEL) > + {} mfbt/STYLE is explicitly indifferent to all-on-one-line versus not-all-on-one-line style for member initialization, so I don't see a reason to change this.

This had to be partially backed out because Win7 debug xpcshell was perma-purple in test_tcpsocket_ipc.js. https://hg.mozilla.org/mozilla-central/rev/5c82f5a5e90d https://tbpl.mozilla.org/php/getParsedLog.php?id=16427937&tree=Mozilla-Inbound Thread 0 (crashed) 0 xul.dll!mozilla::LinkedList<mozilla::ClearOnShutdown_Internal::ShutdownObserver>::~LinkedList<mozilla::ClearOnShutdown_Internal::ShutdownObserver>() [LinkedList.h:fdbf73f25fba : 258 + 0x20] eip = 0x6a2e88cc esp = 0x001ecae0 ebp = 0x001ecb1c ebx = 0x00359b20 esi = 0x6ae3e134 edi = 0x00000001 eax = 0x00000000 ecx = 0x8e40b4bd edx = 0x6d1ce4d8 efl = 0x00000212 Found by: given as instruction pointer in context 1 xul.dll!_CRT_INIT [crtdll.c : 413 + 0x2] eip = 0x6a6b7294 esp = 0x001ecae8 ebp = 0x001ecb1c Found by: call frame info 2 xul.dll!__DllMainCRTStartup [crtdll.c : 526 + 0x10] eip = 0x6a6b74a0 esp = 0x001ecb24 ebp = 0x001ecb60 Found by: call frame info 3 xul.dll!_DllMainCRTStartup [crtdll.c : 476 + 0x10] eip = 0x6a6b7361 esp = 0x001ecb68 ebp = 0x001ecb74 Found by: call frame info 4 ntdll.dll + 0x5af23 eip = 0x76eeaf24 esp = 0x001ecb7c ebp = 0x001ecb94 Found by: call frame info 5 ntdll.dll + 0x63851 eip = 0x76ef3852 esp = 0x001ecb9c ebp = 0x001ecc38 Found by: previous frame's frame pointer 6 ntdll.dll + 0x637c4 eip = 0x76ef37c5 esp = 0x001ecc40 ebp = 0x001ecc4c Found by: previous frame's frame pointer 7 kernel32.dll + 0x52ae3 eip = 0x768b2ae4 esp = 0x001ecc54 ebp = 0x001ecc60 Found by: previous frame's frame pointer 8 MSVCR100D.dll + 0x485ba eip = 0x6d0b85bb esp = 0x001ecc68 ebp = 0x001ecc6c Found by: previous frame's frame pointer 9 MSVCR100D.dll + 0x48466 eip = 0x6d0b8467 esp = 0x001ecc74 ebp = 0x001eccbc Found by: previous frame's frame pointer 10 MSVCR100D.dll + 0x480d1 eip = 0x6d0b80d2 esp = 0x001eccc4 ebp = 0x001eccd0 Found by: previous frame's frame pointer 11 xul.dll!mozilla::dom::ContentChild::QuickExit() [ContentChild.cpp:fdbf73f25fba : 853 + 0x7] eip = 0x6a03e839 esp = 0x001eccd8 ebp = 0x001ecce0 Found by: previous frame's frame pointer 12 xul.dll!mozilla::dom::ContentChild::ProcessingError(mozilla::ipc::HasResultCodes::Result) [ContentChild.cpp:fdbf73f25fba : 834 + 0x4] eip = 0x6a03ed9e esp = 0x001ecce0 ebp = 0x001ecce0 Found by: call frame info 13 xul.dll!mozilla::ipc::AsyncChannel::ReportConnectionError(char const *) [AsyncChannel.cpp:fdbf73f25fba : 645 + 0xc] eip = 0x6a06fc9d esp = 0x001ecce8 ebp = 0x001eccf4 Found by: call frame info 14 xul.dll!mozilla::ipc::RPCChannel::OnMaybeDequeueOne() [RPCChannel.cpp:fdbf73f25fba : 374 + 0xb] eip = 0x6a07cd20 esp = 0x001eccfc ebp = 0x001ecd64 Found by: call frame info 15 xul.dll!MessageLoop::RunTask(Task *) [message_loop.cc:fdbf73f25fba : 333 + 0xd] eip = 0x6a31150d esp = 0x001ecd6c ebp = 0x001ecd84 Found by: call frame info 16 xul.dll!MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &) [message_loop.cc:fdbf73f25fba : 341 + 0x6] eip = 0x6a314baa esp = 0x001ecd8c ebp = 0x001ecd90 Found by: call frame info 17 xul.dll!MessageLoop::DoWork() [message_loop.cc:fdbf73f25fba : 441 + 0x4] eip = 0x6a3154f9 esp = 0x001ecd98 ebp = 0x001ecdc0 Found by: call frame info 18 xul.dll!mozilla::ipc::DoWorkRunnable::Run() [MessagePump.cpp:fdbf73f25fba : 42 + 0x6] eip = 0x6a075bf9 esp = 0x001ecdc8 ebp = 0x001ecdd0 Found by: call frame info 19 xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:fdbf73f25fba : 620 + 0xd] eip = 0x6a2d0e06 esp = 0x001ecdd8 ebp = 0x001ece04 Found by: call frame info 20 xul.dll!NS_ProcessNextEvent_P(nsIThread *,bool) [nsThreadUtils.cpp:fdbf73f25fba : 220 + 0xc] eip = 0x6a27f39d esp = 0x001ece0c ebp = 0x001ece18 Found by: call frame info 21 xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [MessagePump.cpp:fdbf73f25fba : 117 + 0x9] eip = 0x6a076011 esp = 0x001ece20 ebp = 0x001ece44 Found by: call frame info 22 xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [MessagePump.cpp:fdbf73f25fba : 231 + 0x8] eip = 0x6a076297 esp = 0x001ece4c ebp = 0x001ece5c Found by: call frame info 23 xul.dll!MessageLoop::RunInternal() [message_loop.cc:fdbf73f25fba : 215 + 0x8] eip = 0x6a3135a0 esp = 0x001ece64 ebp = 0x001ece7c Found by: call frame info 24 xul.dll!MessageLoop::RunHandler() [message_loop.cc:fdbf73f25fba : 208 + 0x4] eip = 0x6a313c1b esp = 0x001ece84 ebp = 0x001eceb0 Found by: call frame info 25 xul.dll!MessageLoop::Run() [message_loop.cc:fdbf73f25fba : 182 + 0x6] eip = 0x6a31421f esp = 0x001eceb8 ebp = 0x001eced0 Found by: call frame info 26 xul.dll!nsBaseAppShell::Run() [nsBaseAppShell.cpp:fdbf73f25fba : 163 + 0xb] eip = 0x69f65878 esp = 0x001eced8 ebp = 0x001ecee0 Found by: call frame info 27 xul.dll!nsAppShell::Run() [nsAppShell.cpp:fdbf73f25fba : 232 + 0x5] eip = 0x69f23272 esp = 0x001ecee8 ebp = 0x001eee34 Found by: call frame info 28 xul.dll!XRE_RunAppShell [nsEmbedFunctions.cpp:fdbf73f25fba : 646 + 0xd] eip = 0x68fe70a1 esp = 0x001eee3c ebp = 0x001eee48 Found by: call frame info 29 xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [MessagePump.cpp:fdbf73f25fba : 198 + 0x4] eip = 0x6a0761e8 esp = 0x001eee50 ebp = 0x001eee60 Found by: call frame info 30 xul.dll!MessageLoop::RunInternal() [message_loop.cc:fdbf73f25fba : 215 + 0x8] eip = 0x6a3135a0 esp = 0x001eee68 ebp = 0x001eee80 Found by: call frame info 31 xul.dll!MessageLoop::RunHandler() [message_loop.cc:fdbf73f25fba : 208 + 0x4] eip = 0x6a313c1b esp = 0x001eee88 ebp = 0x001eeeb4 Found by: call frame info 32 xul.dll!MessageLoop::Run() [message_loop.cc:fdbf73f25fba : 182 + 0x6] eip = 0x6a31421f esp = 0x001eeebc ebp = 0x001eeed4 Found by: call frame info 33 xul.dll!XRE_InitChildProcess [nsEmbedFunctions.cpp:fdbf73f25fba : 485 + 0xa] eip = 0x68fe6ee5 esp = 0x001eeedc ebp = 0x001ef880 Found by: call frame info 34 plugin-container.exe!NS_internal_main(int,char * *) [MozillaRuntimeMain.cpp:fdbf73f25fba : 48 + 0xa] eip = 0x00cf142c esp = 0x001ef888 ebp = 0x001ef89c Found by: call frame info 35 plugin-container.exe!wmain [nsWindowsWMain.cpp:fdbf73f25fba : 105 + 0x6] eip = 0x00cf1594 esp = 0x001ef8a4 ebp = 0x001ef8d4 Found by: call frame info 36 plugin-container.exe!__tmainCRTStartup [crtexe.c : 552 + 0x18] eip = 0x00cf191f esp = 0x001ef8dc ebp = 0x001ef924 Found by: call frame info 37 plugin-container.exe!wmainCRTStartup [crtexe.c : 370 + 0x4] eip = 0x00cf174f esp = 0x001ef92c ebp = 0x001ef92c Found by: call frame info 38 kernel32.dll + 0x51173 eip = 0x768b1174 esp = 0x001ef934 ebp = 0x001ef938 Found by: call frame info 39 ntdll.dll + 0x5b3f4 eip = 0x76eeb3f5 esp = 0x001ef940 ebp = 0x001ef978 Found by: previous frame's frame pointer 40 ntdll.dll + 0x5b3c7 eip = 0x76eeb3c8 esp = 0x001ef980 ebp = 0x001ef990 Found by: previous frame's frame pointer

We can put this assertion back in once bug 805207 is reviewed. Although I suspect that something bad is actually happening in this test, and the LinkedList code just happens to be pointing it out: 12 xul.dll!mozilla::dom::ContentChild::ProcessingError(mozilla::ipc::HasResultCodes::Result) 13 xul.dll!mozilla::ipc::AsyncChannel::ReportConnectionError(char const *)

Backed-out part re-landed https://hg.mozilla.org/integration/mozilla-inbound/rev/74e7f7678c88

(In reply to Justin Lebar [:jlebar] from comment #8) > Backed-out part re-landed > https://hg.mozilla.org/integration/mozilla-inbound/rev/74e7f7678c88 https://hg.mozilla.org/mozilla-central/rev/74e7f7678c88

Comment on attachment 673406 [details] [diff] [review] Patch, v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 836654 User impact if declined: Bug 844422 Testing completed: On m-c for a while Risk to taking this patch (and alternatives if risky): This makes LinkedList safer. We could fix bug 844422 directly, but I think it's much better to have consistent semantics across our branches, so we avoid this problem in the future. String or UUID changes made by this patch: none NOTE: We need either this patch without the following change > + ~LinkedList() { > + MOZ_ASSERT(isEmpty()); > + } or this patch plus the patch in bug 805207. I'm happy if we simply remove this MOZ_ASSERT when we land on b2g18.

This tef+ bug needs to land alongside bug 836654.

(In reply to Justin Lebar [:jlebar] from comment #10) > NOTE: We need either this patch without the following change > > > + ~LinkedList() { > > + MOZ_ASSERT(isEmpty()); > > + } > > or this patch plus the patch in bug 805207. I'm happy if we simply remove > this MOZ_ASSERT when we land on b2g18. Let's go with less code change (let's not take bug 805207).

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d683eefee3f0 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/819504e27351 The Gecko bits of bug 836654 already landed on b2g18_v1_0_1 (see bug 836654 comment 65). The status flag was set back to affected for some Gaia-specific bits that need to land.