Fixing crashes, code cleaning, and features! by CodeforEvolution · Pull Request #34 · HaikuArchives/StreamRadio

CodeforEvolution · 2020-09-01T03:02:58Z

I think I got carried away... :)

Quite a few things: - Code formatting - More checks for failing functions and NULL - We look for B_SIMPLE_DATA (it's the same format as B_REFS_RECEIVED) - Added automatic size limits to the MainWindow - Removed unused "Buffering.h" and unused function "MainWindow::SetVisible" Should fix HaikuArchives#33, HaikuArchives#32, HaikuArchives#29

humdingerb

Looks good, commented just for an explanation...

Also, in the future you may want to put all code style changes in its own commit (within the one PR). Makes sorting out the functional changes easier.

humdingerb · 2020-09-01T06:07:16Z

 						= fSettings->Stations->FindItem(station->Name());
-					if (existingStation) {
+					if (existingStation != NULL) {
 						delete station;


Just for my education:

why is object 'station' destroyed via "delete" when it wasn't created with "new"?

how can the object 'station' that was 'deleted' in the previous line get a new value assigned?
I ask genuinely. I really am a crappy 'coder'...

I must admit, I need to work on separating my style changes. :)

And hey, no worries! I'm happy to explain:

While it may look like the station variable isn't allocated with "new", you'll want to look above at this line:
Station* station = Station::Load(ref.name, new BEntry(&ref));

We'll want to look in the Station.cpp file and find the function that's being called above:

StreamRadio/Station.cpp

Line 438 in c654e0c

Station::Load(BString name, BEntry* entry)

We can see that in this function, a Station object is allocated with "new":

StreamRadio/Station.cpp

Line 443 in c654e0c

Station* station = new Station(name);

Eventually, if all goes well in the Load() function, we are returned the fresh "Station" object that was created by "new":

StreamRadio/Station.cpp

Line 551 in c654e0c

return station;

As a result, the station variable is assigned to the Station object that was created with "new".

As for the immediate reassignment, you'll want to check out this video about pointers:
https://youtu.be/DTxHyVn0ODg

He also makes a bunch of other great videos on C++ too:
https://www.youtube.com/playlist?list=PLlrATfBNZ98dudnM48yfGUldqGD0S4FFb

Thanks for the explanations!
I didn't find anything on "immediate reassignment" in that video. I sat through the 15 minutes of it though and would advise the guy to keep away from the coffee while recording. :)

No problem! On the other hand, you get a lot of info and in a short amount of time! ;)

CodeforEvolution · 2020-09-02T00:39:12Z

Hold up...one quick problem that I introduced that needs to be fixed...

humdingerb · 2020-09-06T06:50:12Z

@CodeforEvolution, ready to be merged? Or, what's the quick problem?

Vidrep · 2020-09-06T18:37:54Z

humdinger, I've been using this brach for a few days now. Not a single crash. Maybe just a couple of tweaks to the GUI are all that's needed now.

CodeforEvolution · 2020-09-06T21:50:35Z

Unfortunately this is still not mergeable, even though the program isn’t crashing, without that “delete”, StreamRadio will start leaking network threads and audio resources when switching between stations. I just don’t have the time to look into this further...though I think it might be BLocker related?

humdingerb · 2020-09-07T15:06:56Z

Can you link to the specific location(s) in the code that's the issue? Not that I can fix it, most probably, but maybe someone else can... :)

Are the leaks that severe that it impacts usage so significantly, that we shouldn't release these PRs improvements?

CodeforEvolution · 2020-09-07T16:47:07Z

https://github.com/CodeforEvolution/StreamRadio/blob/029dbd4c1212533b39cbe73c42f79db5507bc448/MainWindow.cpp#L321
Before my changes, the "player" variable was deleted right above this line of code, which deleted an instance of the "StreamPlayer" class (a class that handles playback for a single station). This "delete" was causing a crash for me (and it seems for you guys too) that occurred after playing and pausing any station two times. The crash had to do with BLocker when it was trying to be destructed (which StreamPlayer is based upon). But the problem is pretty simple if we don't use delete: The StreamPlayer is leaked. This is because once the associated "StationItem" instance is unmapped from it, nothing else is keeping track of the StreamPlayer, and thus its resources, including a network thread that was used for reading the audio data, is leaked.

Vidrep · 2020-10-05T23:27:21Z

Playing around with Cppcheck, so decided to run it on MainWindow.cpp. Result below. Hope it helps.

~/HaikuArchives/StreamRadio-crashEtClean> cppcheck --enable=all MainWindow.cpp
Checking MainWindow.cpp ...
[MainWindow.cpp:53]: (style) C-style pointer casting
[StationListView.h:41]: (style) Class 'StationListViewItem' has a constructor with 1 argument that is not explicit.
[StationListView.h:74]: (style) Class 'StationListView' has a constructor with 1 argument that is not explicit.
[StationFinder.h:74]: (style) Class 'FindByCapability' has a constructor with 1 argument that is not explicit.
[StationFinder.h:132]: (style) Class 'StationFinderWindow' has a constructor with 1 argument that is not explicit.
Checking MainWindow.cpp: DEBUGGING...
Checking MainWindow.cpp: PROFILING...
Checking MainWindow.cpp: TRACING...
[MainWindow.cpp:393]: (style) The function 'QuitRequested' is never used.

jsteinaker · 2021-02-20T03:03:15Z

Sorry about not quoting and stuff. I might have discovered what's happening here (but then again, I don't know jack about C++ so...). On the StreamPlayer class destructor, I've just commented the first line, which posts the MSG_PLAYER_STATE_CHANGED message (Stopped). If you take a look at MainWindow.cpp, when the window receives a message like that triggers the delete routine again, which in turn calls the destructor, which will try to post the message again, thus causing a loop or double-delete

This change (plus the others) is already on PR. Can someone give it a try and confirm if it's working (and correct me if I'm wrong about what I think it's happening)?

humdingerb · 2021-03-08T18:25:01Z

This change (plus the others) is already on PR. Can someone give it a try and confirm if it's working (and correct me if I'm wrong about what I think it's happening)?

Just to clarify, the PR you ask to try is #39, right? (Pulkomandy and I alredy commented there.)

humdingerb reviewed Sep 1, 2020

View reviewed changes

CodeforEvolution marked this pull request as draft September 2, 2020 00:39

kenmays mentioned this pull request Oct 13, 2020

Prepare for a Release...Soon! #27

Open

humdingerb mentioned this pull request Nov 23, 2021

Posible memory leak #48

Closed

Uh oh!

Conversation

CodeforEvolution commented Sep 1, 2020

Uh oh!

humdingerb left a comment

Choose a reason for hiding this comment

Uh oh!

humdingerb Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

CodeforEvolution Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeforEvolution Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

humdingerb Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

CodeforEvolution Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

CodeforEvolution commented Sep 2, 2020

Uh oh!

humdingerb commented Sep 6, 2020

Uh oh!

Vidrep commented Sep 6, 2020

Uh oh!

CodeforEvolution commented Sep 6, 2020

Uh oh!

humdingerb commented Sep 7, 2020

Uh oh!

CodeforEvolution commented Sep 7, 2020

Uh oh!

Vidrep commented Oct 5, 2020

Uh oh!

jsteinaker commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humdingerb commented Mar 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

CodeforEvolution Sep 1, 2020 •

edited

Loading

jsteinaker commented Feb 20, 2021 •

edited

Loading