803688 - Remove LinkedListElements from their list when they're destructed, and assert that a LinkedList is empty when it's destructed

Assignee

Description

•

11 years ago

These changes will make mozilla::LinkedList a bit safer.

Patch in a moment.

Justin Lebar (not reading bugmail)

Assignee

Comment 1

•

11 years ago

Attached patch Patch, v1 — Details — Splinter Review

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.

Attachment #673406 - Flags: review?(jwalden+bmo)

Justin Lebar (not reading bugmail)

Assignee

Updated

•

11 years ago

Assignee: nobody → justin.lebar+bug

Justin Lebar (not reading bugmail)

Assignee

Comment 2

•

11 years ago

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

Jeff Walden [:Waldo]

Comment 3

•

11 years ago

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.

Attachment #673406 - Flags: review?(jwalden+bmo) → review+

Justin Lebar (not reading bugmail)

Assignee

Comment 4

•

11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/592c3465a742

Ryan VanderMeulen [:RyanVM]

Comment 5

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/592c3465a742

Status: NEW → RESOLVED

Closed: 11 years ago

Flags: in-testsuite-

Resolution: --- → FIXED

Target Milestone: --- → mozilla19

Ryan VanderMeulen [:RyanVM]

Comment 6

•

11 years ago

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

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Justin Lebar (not reading bugmail)

Assignee

Comment 7

•

11 years ago

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 *)

Justin Lebar (not reading bugmail)

Assignee

Updated

•

11 years ago

Depends on: 805207

Justin Lebar (not reading bugmail)

Assignee

Comment 8

•

11 years ago

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

Ed Morley [:emorley]

Comment 9

•

11 years ago

(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

Status: REOPENED → RESOLVED

Closed: 11 years ago → 11 years ago

Resolution: --- → FIXED

Ian Neal

Updated

•

11 years ago

Blocks: 808379

Justin Lebar (not reading bugmail)

Assignee

Comment 10

•

10 years ago

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.

Attachment #673406 - Flags: approval-mozilla-b2g18?

Alex Keybl [:akeybl]

Updated

•

10 years ago

Attachment #673406 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

Alex Keybl [:akeybl]

Updated

•

10 years ago

blocking-b2g: --- → tef+

Alex Keybl [:akeybl]

Comment 11

•

10 years ago

This tef+ bug needs to land alongside bug 836654.

Whiteboard: [Land with bug 836654]

Alex Keybl [:akeybl]

Comment 12

•

10 years ago

(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).

Ryan VanderMeulen [:RyanVM]

Comment 13

•

10 years ago

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.

status-b2g18: --- → fixed

status-b2g18-v1.0.0: --- → wontfix

status-b2g18-v1.0.1: --- → fixed

status-firefox19: --- → fixed

Whiteboard: [Land with bug 836654]

Daniel Veditz [:dveditz]

Updated

•

10 years ago

Blocks: 844422

Bugzilla

Quick Search

Remove LinkedListElements from their list when they're destructed, and assert that a LinkedList is empty when it's destructed

Categories

(Core :: MFBT, defect)

Tracking

()

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Updated

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Updated

Comment 8

Comment 9

Updated

Comment 10

Updated

Updated

Comment 11

Comment 12

Comment 13

Updated