If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove more PL_DHashTable{Init,Finish}() calls

RESOLVED FIXED in Firefox 41

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Like bug 1166586. These ones are marginally trickier, mostly involving a call to PL_DHashTableInit() in an Init() function. But still pretty straightforward.
(Assignee)

Comment 1

2 years ago
Created attachment 8608432 [details] [diff] [review]
(part 1) - Use PLDHashTable2 in nsScriptNameSpaceManager
Attachment #8608432 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter
Attachment #8608433 - Flags: review?(nfroyd)
(Assignee)

Comment 3

2 years ago
Created attachment 8608434 [details] [diff] [review]
(part 3) - Use PLDHashTable2 in nsCommandParams
Attachment #8608434 - Flags: review?(nfroyd)
(Assignee)

Comment 4

2 years ago
Created attachment 8608436 [details] [diff] [review]
(part 4) - Use PLDHashTable2 in RDFServiceImpl
Attachment #8608436 - Flags: review?(nfroyd)
(Assignee)

Comment 5

2 years ago
Created attachment 8608437 [details] [diff] [review]
(part 5) - Use PLDHashTable2 in InMemoryDataSource
Attachment #8608437 - Flags: review?(nfroyd)
(Assignee)

Comment 6

2 years ago
Created attachment 8608439 [details] [diff] [review]
(part 6) - Clean up nsStaticCaseInsensitiveNameTable
Attachment #8608439 - Flags: review?(nfroyd)
(Assignee)

Comment 7

2 years ago
Created attachment 8608440 [details] [diff] [review]
(part 7) - Use PLDHashTable2 in nsLoadGroup
Attachment #8608440 - Flags: review?(nfroyd)
(Assignee)

Comment 8

2 years ago
Created attachment 8608441 [details] [diff] [review]
(part 8) - Use PLDHashTable2 in nsHostResolver
Attachment #8608441 - Flags: review?(nfroyd)
(Assignee)

Comment 9

2 years ago
Created attachment 8608442 [details] [diff] [review]
(part 9) - Use PLDHashTable2 in nsTHashtable
Attachment #8608442 - Flags: review?(nfroyd)
Attachment #8608432 - Flags: review?(nfroyd) → review+
Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

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

r=me with either splitting out the bit below to a different bug or getting somebody to comment on the goodness of it.

::: layout/tables/SpanningCellSorter.cpp
@@ -150,5 @@
>                  HashTableEntry **sh =
>                      new HashTableEntry*[mHashTable.EntryCount()];
> -                if (!sh) {
> -                    // give up
> -                    mState = DONE;

Clearly this can be removed in its current state, but I think it'd be worth it to get a layout person to confirm that we want to infallibly allocate here, since the hunks that I can see seem to make some effort towards handling allocation failures gracefully.
Attachment #8608433 - Flags: review?(nfroyd) → review+
Attachment #8608434 - Flags: review?(nfroyd) → review+
Attachment #8608436 - Flags: review?(nfroyd) → review+
Comment on attachment 8608437 [details] [diff] [review]
(part 5) - Use PLDHashTable2 in InMemoryDataSource

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

::: rdf/base/nsInMemoryDataSource.cpp
@@ +754,5 @@
>      NS_ADDREF(datasource);
>  
> +    datasource->fAggregated.AddRef();
> +    nsresult rv = datasource->AggregatedQueryInterface(aIID, aResult); // This'll AddRef()
> +    datasource->fAggregated.Release();

This code...
Attachment #8608437 - Flags: review?(nfroyd) → review+
Comment on attachment 8608440 [details] [diff] [review]
(part 7) - Use PLDHashTable2 in nsLoadGroup

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

::: netwerk/base/nsLoadGroup.h
@@ +49,5 @@
>      ////////////////////////////////////////////////////////////////////////////
>      // nsLoadGroup methods:
>  
>      explicit nsLoadGroup(nsISupports* outer);
> +    virtual ~nsLoadGroup();

Um.  I guess this doesn't hit the public-destructor checks in nsISupportsImpl.h because we use a different set of macros to define AddRef/Release for this class?

Can you file followup bugs for:

a) adding the same MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING to the macros in nsAgg.h;
b) making NS_GENERIC_AGGREGATED_CONSTRUCTOR not require access to the destructor?  (I think we can do this by use the same idea as NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT.).

These aren't terribly important bugs, as there's not that many aggregated classes in m-c (part (a) above might break comm-central, though), but a little more safety never hurt anybody.
Attachment #8608440 - Flags: review?(nfroyd) → review+
Attachment #8608441 - Flags: review?(nfroyd) → review+
Attachment #8608442 - Flags: review?(nfroyd) → review+
Comment on attachment 8608439 [details] [diff] [review]
(part 6) - Clean up nsStaticCaseInsensitiveNameTable

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

I think this could have been several patches, but whatever.
Attachment #8608439 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 14

2 years ago
Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

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

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10)
> Comment on attachment 8608433 [details] [diff] [review]
> (part 2) - Use PLDHashTable2 in SpanningCellSorter
> 
> Review of attachment 8608433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with either splitting out the bit below to a different bug or getting
> somebody to comment on the goodness of it.
> 
> ::: layout/tables/SpanningCellSorter.cpp
> @@ -150,5 @@
> >                  HashTableEntry **sh =
> >                      new HashTableEntry*[mHashTable.EntryCount()];
> > -                if (!sh) {
> > -                    // give up
> > -                    mState = DONE;
> 
> Clearly this can be removed in its current state, but I think it'd be worth
> it to get a layout person to confirm that we want to infallibly allocate
> here, since the hunks that I can see seem to make some effort towards
> handling allocation failures gracefully.

dholbert, would you mind taking a look? Thanks.
Attachment #8608433 - Flags: review?(dholbert)
Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

Yeah, it looks like we might really want fallible allocation there.

I'm unfamiliar with SpanningCellSorter; I'll punt to dbaron, who wrote this code (ages ago) according to https://hg.mozilla.org/users/gszorc_mozilla.com/gecko-full/annotate/c9de915dc050/layout/tables/SpanningCellSorter.cpp#l152

(dbaron, see comment 10.)
Attachment #8608433 - Flags: review?(dholbert) → review?(dbaron)
I don't think the OOM-handling done by SpanningCellSorter (and the lack of result-checking by AddCell's caller) amounts to any useful OOM-handling, so I think it should probably be infallible.
Comment on attachment 8608433 [details] [diff] [review]
(part 2) - Use PLDHashTable2 in SpanningCellSorter

r=dbaron, although my one comment would be that in most cases in the current code this hash table will *not* get initialized.  It will only get initialized if the table has a cell with colspan >= 10.  So my only concern here would not be the infallible allocation (which is fine, see previous comment), but any increased costs of initialization and destruction.  Maybe they're too small to worry about, though.

(The whole point of SpanningCellSorter is to enumerate all cells with colspans in a table, sorted by their colspan.)
Attachment #8608433 - Flags: review?(dbaron) → review+
(Assignee)

Comment 18

2 years ago
> r=dbaron, although my one comment would be that in most cases in the current
> code this hash table will *not* get initialized.  It will only get
> initialized if the table has a cell with colspan >= 10.  So my only concern
> here would ... be ... any increased costs of initialization and destruction.  Maybe
> they're too small to worry about, though.

I just did some simple instrumentation and in a couple of minutes of browsing across a few sites only a few thousand SpanningCellSorter classes were created. Seems few enough to not cause a problem. If it does, allocating the hash table on the heap will be an option.
(Assignee)

Comment 19

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #9)
> Created attachment 8608442 [details] [diff] [review]
> (part 9) - Use PLDHashTable2 in nsTHashtable

Well, this is odd. This patch works fine on all platforms but Windows, where it lights up Treeherder like a Skittles commercial: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4186e372a1c8

I may land the first eight patches and leave this one for later.
(Assignee)

Comment 20

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #19)
> >
> > Created attachment 8608442 [details] [diff] [review]
> > (part 9) - Use PLDHashTable2 in nsTHashtable
> 
> Well, this is odd. This patch works fine on all platforms but Windows, where
> it lights up Treeherder like a Skittles commercial:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4186e372a1c8

I did some local Windows builds and I've determined that it's the movement of |sOps| from a function to the class that's the problem; it's *not* the change of |mTable| to a PLDHashTable2. Specifically, Firefox builds with the |sOps| change simply fail to start, and debug builds give this assertion failure:

> Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

WindowsCrtPatch.h is one of those special super hacks we have, and I don't claim to understand it at all. But somehow, having a templatized static class member is breaking it.

dmajor, any ideas why? Thank you.
Flags: needinfo?(dmajor)

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/df1d66a31697
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eaac66f8bbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe49838e8aba
https://hg.mozilla.org/integration/mozilla-inbound/rev/b373d9cab11f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9cc200b499
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b79df6cb61
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eaaca25256
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5226ab4feca
(Assignee)

Comment 22

2 years ago
I landed the first 8 patches.

> I did some local Windows builds and I've determined that it's the movement of |sOps| from a function
> to the class that's the problem

It looks like if I move |sOps| into a static function, that's enough to get past the WindowsCrtPatch.h problem. But I'll do that in another bug.
(Assignee)

Comment 23

2 years ago
> Can you file followup bugs for:
> 
> a) adding the same MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING to the macros in
> nsAgg.h;
> b) making NS_GENERIC_AGGREGATED_CONSTRUCTOR not require access to the
> destructor?  (I think we can do this by use the same idea as
> NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT.).

Done: bug 1168006.
(Assignee)

Updated

2 years ago
Attachment #8608442 - Attachment is obsolete: true
> > Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

Yikes! I'm glad the asserts caught this, and thanks for flagging me. Even if you've found a workaround, I'd like to build this locally tomorrow and see what's going on.
https://hg.mozilla.org/mozilla-central/rev/df1d66a31697
https://hg.mozilla.org/mozilla-central/rev/7eaac66f8bbe
https://hg.mozilla.org/mozilla-central/rev/fe49838e8aba
https://hg.mozilla.org/mozilla-central/rev/b373d9cab11f
https://hg.mozilla.org/mozilla-central/rev/5a9cc200b499
https://hg.mozilla.org/mozilla-central/rev/d2b79df6cb61
https://hg.mozilla.org/mozilla-central/rev/b9eaaca25256
https://hg.mozilla.org/mozilla-central/rev/e5226ab4feca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
> Assertion failure: !GetModuleHandleA("mozglue.dll"), at c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\toolkit\\xre\\WindowsCrtPatch.h:126

For some reason part 9 was provoking the issue in bug 1023941 comment 32. I don't know why the xpcom directory is so susceptible to this. I tried to investigate this once before, in bug 1159416 comment 4, but I didn't get anywhere.
Flags: needinfo?(dmajor)
You need to log in before you can comment on or make changes to this bug.