1157728 - Useless call in /layout/base/StackArena.h

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

https://dxr.mozilla.org/mozilla-central/source/layout/base/StackArena.h?from=StackArena.h#73

gStackArena->Init(); 
has no effect and should be removed

Init() being only:
  nsresult Init() { return mBlocks ? NS_OK : NS_ERROR_OUT_OF_MEMORY; }

Comment 1

[Ryan Nath [:rnath]]

Hi Sylvestre, I would like to work on this bug!

Comment 2

[Shyam Venkatesh]

Hey I'd love to work on this bug and I already started. After removing the line specified and running xpcshell on it the following errors were produced.

Summary
=======

Ran 2281 tests (2262 parents, 19 subtests)
Expected results: 2176
Unexpected results: 6 (FAIL: 6)
Skipped: 99

Unexpected Results
==================

js/xpconnect/tests/unit/test_file2.js
-------------------------------------
FAIL [Parent]
js/xpconnect/tests/unit/test_file.js
------------------------------------
FAIL [Parent]
modules/libjar/test/unit/test_bug407303.js
------------------------------------------
FAIL [Parent]
toolkit/components/places/tests/queries/test_sort-date-site-grouping.js
-----------------------------------------------------------------------

Comment 3

[Shyam Venkatesh]

Running the build seems to work fine. Maybe the fails are unrelated?

Comment 4

[Shyam Venkatesh]

Attachment: Removed the useless call specified.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8597157 [details] [diff] [review]
Removed the useless call specified.

You have to create a diff, not copy the full file.
See:
https://developer.mozilla.org/fr/docs/Mercurial_Queues#How_to_use_MQ_for_Mozilla_development

Comment 6

[Shyam Venkatesh]

Attachment: Created a patch file for the change.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Please remove the "browser/config/mozconfig" part (and you should use .mozconfig for this) from the patch. Besides that, it is fine.

Comment 8

[Shyam Venkatesh]

Attachment: bug-1157728-fix.patch

Comment 9

[Sylvestre Ledru [:Sylvestre]]

OK. Good! Now, try to find who can review this change. 
Trick:
In this history of the file, look who reviewed or wrote patches on this file.

Comment 10

[Shyam Venkatesh]

https://hg.mozilla.org/mozilla-central/annotate/22a157f7feb7/layout/base/StackArena.h

From here it seems either the author Ryan VanderMeulen or matspal.

Comment 11

[Nathan Froyd [:froydnj]]

matspal or dholbert would be fine choices.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Don't use the annotate, if it is not really efficient for this.

The history itself is better:
https://hg.mozilla.org/mozilla-central/filelog/22a157f7feb7/layout/base/StackArena.h

Comment 13

[Shyam Venkatesh]

Did I do it right?

Comment 14

[Sylvestre Ledru [:Sylvestre]]

You did right!

Comment 15

[Shyam Venkatesh]

Great, that was my first bug :D
Thanks for all the help Sylvestre :)

Comment 16

[Sylvestre Ledru [:Sylvestre]]

(In reply to Shyam Venkatesh from comment #15)
> Great, that was my first bug :D
Oh, that is not over ;) Daniel might have comments & we have to land the patch once it is reviewed (r+)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8597251 [details] [diff] [review]
bug-1157728-fix.patch

Thanks for taking the bug!

Two requests:
 (1) Could you remove the Init() function itself?  Nobody calls it after this point, so it's dead code. (And long as an Init() function exists, we technically *should* be calling it before using the object [to do whatever "initialization work" that object needs]. In case case it's useless, as Sylvestre points out -- but in the future, someone could conceivably add more initialization to Init() and expect that it'll be run. To prevent that kind of misunderstanding, we should just remove it.)

 (2) Please make your commit message a bit more specific. It's currently:
>Bug 1157728: Removed the useless function call.
Better would be something like:
  Bug 1157728: Remove StackArena::Init(), because it doesn't do anything. r=dholbert
(This is more specific now, per (1), you'll be removing the function itself.)

If you repost a patch with those tweaks, I'll mark it r+. Thanks!

Comment 18

[Daniel Holbert [:dholbert]]

(Some history here: it looks like the point of Init() was to let us determine whether the constructor succeeded in allocating memory for mBlocks. That's no longer necessary, though, because ever since a few years ago, our memory-allocation is infallible -- the allocator simply aborts if it fails, in most cases. So we can assume that "mBlocks" is initialized after the constructor runs, which means Init() is unnecessary.)

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (And long as an Init() function exists, we
> technically *should* be calling it before using the object [to do whatever
> "initialization work" that object needs]. In case case it's useless

(Sorry for typos; I meant "As long as...", and "In this case...".  Clearly I shouldn't try to type first thing in the morning. :))

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8597251 [details] [diff] [review]
bug-1157728-fix.patch

Marking first patch revision "r-", since it's not quite ready to land.

(Updated version will be r+, w/ comment 17 addressed -- just tag me for review when you re-post.)

Comment 21

[Shyam Venkatesh]

Attachment: Removed StackArena::Init() as well.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8597565 [details] [diff] [review]
Removed StackArena::Init() as well.

Thanks! r=me

Comment 23

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/330a2ad36545

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/330a2ad36545