[opensource-dev] Review Request: Fix for viewer crash when entering anything but a valid dns-address into the grid selection combo (login screen).

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Review request for Viewer. By MartinRJ Fayray. Description FIX FOR VWR-23741 (https://jira.secondlife.com/browse/vwr-23741) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/vwr-23741 About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: VWR-23741 Diffs indra/llcommon/llversionviewer.h (e9c82fca5ae6) View Diff

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Review request for Viewer. By MartinRJ Fayray.

Updated March 6, 2012, 10:31 p.m. Summary (updated) VWR-23741: Fix for viewer crash when entering anything but a valid dns-address into the grid selection combo (login screen).

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Review request for Viewer. By MartinRJ Fayray.

Updated March 6, 2012, 11:15 p.m.

Description FIX FOR VWR-23741 (https://jira.secondlife.com/browse/vwr-23741) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/vwr-23741 About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: VWR-23741

Diffs (updated) indra/newview/llpanellogin.cpp (0a41a8750048) indra/newview/llviewernetwork.h (0a41a8750048) indra/newview/skins/default/xui/en/notifications.xml (0a41a8750048) View Diff

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Review request for Viewer. By MartinRJ Fayray.

Updated March 24, 2012, 5:33 a.m. Changes Rebuilt as STORM-1818 with repository pulled from viewer-release (updated diff-file). Summary (updated) STORM-1818: Fix for viewer crash when entering chars other than [a-z0-9-_. ] into the grid-selection combo. Description (updated) FIX FOR STORM-1818 (https://jira.secondlife.com/browse/storm-1818) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/storm-1818/

About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files.

Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni).

Bugs: STORM-1818 Diffs (updated) indra/newview/llpanellogin.cpp (acfb0781d850) indra/newview/llviewernetwork.h (acfb0781d850) indra/newview/skins/default/xui/en/notifications.xml (acfb0781d850) View Diff

[Jonathan Yap]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

It seems to me that the better fix would be to support the URLs, as you say some TPVs do, that are currently causing trouble.

- Jonathan

On March 24th, 2012, 5:33 a.m., MartinRJ Fayray wrote:

Review request for Viewer. By MartinRJ Fayray. Updated March 24, 2012, 5:33 a.m.

Description

FIX FOR STORM-1818 (https://jira.secondlife.com/browse/storm-1818) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/storm-1818/ About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: STORM-1818 Diffs

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

On March 24th, 2012, 7:22 a.m., Jonathan Yap wrote:

It seems to me that the better fix would be to support the URLs, as you say some TPVs do, that are currently causing trouble.

You can still login to non-Linden Lab grids via the --login URI parameter https://wiki.secondlife.com/wiki/Viewer_parameters
But the grid selection combo doesn't support adding custom grids with a 'corrupt' format at all, as it seems the whole viewer is NOT designed for non-Linden Lab grids, see here: https://jira.secondlife.com/browse/VWR-28570?focusedCommentId=315423&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-315423

The hack to support these non-Linden Lab grid-format-urls as suggested here is an attempt to try parsing input data of any given format - be it valid or not - and a guessing game what the users input might mean, and that's in my honest opinion more of a 'political' question for the Linden Lab managment, and not a crash-fix-issue: https://jira.secondlife.com/browse/STORM-1818?focusedCommentId=314515&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-314515

Please test the fix/ship it.

Thanks in advance. Kind regards, MartinRJ

- MartinRJ

[Lance Corrimal]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

I would like to point out that having all those linden-internal grids in there is utterly useless unless you are inside the linden lab network. How about adding a proper grid manager that supports actually accessible grids instead?

- Lance

On March 24th, 2012, 5:33 a.m., MartinRJ Fayray wrote:

Review request for Viewer. By MartinRJ Fayray. Updated March 24, 2012, 5:33 a.m.

Description

FIX FOR STORM-1818 (https://jira.secondlife.com/browse/storm-1818) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/storm-1818/ About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: STORM-1818 Diffs

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

On March 24th, 2012, 8:08 a.m., Lance Corrimal wrote:

I would like to point out that having all those linden-internal grids in there is utterly useless unless you are inside the linden lab network. How about adding a proper grid manager that supports actually accessible grids instead?

@Lance this is only a review of my crash fix, no public discussion. It is only about this severe bug (abnormal program termination). This is not a public user group meeting where we discuss features, please bear in mind that you just demanded me - a volunteer contributor - to add a proper grid manager:
"How about adding a proper grid manager"?
The answer is: no! Of course not!

- MartinRJ

[Tofu Buzzard]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Thanks for looking at this issue, MartinRJ.
The only thing that give me pause for thought is - if the crash is coming from an uncaught exception, the 'right' thing to do seems to be to catch the exception and show the notification you've created then, aborting the login.  Any clear reason why this isn't the approach?  Does pre-validation give a better user experience?
Cheers.

- Tofu

On March 24th, 2012, 5:33 a.m., MartinRJ Fayray wrote:

Review request for Viewer. By MartinRJ Fayray. Updated March 24, 2012, 5:33 a.m.

Description

FIX FOR STORM-1818 (https://jira.secondlife.com/browse/storm-1818) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/storm-1818/ About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: STORM-1818 Diffs

[MartinRJ Fayray]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

On March 25th, 2012, 3:13 a.m., Tofu Buzzard wrote:

Thanks for looking at this issue, MartinRJ.
The only thing that give me pause for thought is - if the crash is coming from an uncaught exception, the 'right' thing to do seems to be to catch the exception and show the notification you've created then, aborting the login.  Any clear reason why this isn't the approach?  Does pre-validation give a better user experience?
Cheers.

@Tofu: you want to remove the un-supported input string from the combo-box and have a valid grid selected instead.
Otherwise, if you'd do what you've mentioned what would be the 'right' thing, and only catch the exception, if the user presses Login after that notification he'd get the next error.

- MartinRJ

[Tofu Buzzard]

This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/563/

Ship it!

Ah, that makes things much clearer.  Yes, that would be lame.

- Tofu

On March 24th, 2012, 5:33 a.m., MartinRJ Fayray wrote:

Review request for Viewer. By MartinRJ Fayray. Updated March 24, 2012, 5:33 a.m.

Description

FIX FOR STORM-1818 (https://jira.secondlife.com/browse/storm-1818) "Abnormal program termination when I enter an URL to the grid selection combobox (login-screen) and press enter." Repository with fix: https://bitbucket.org/MartinRJ/storm-1818/ About: When entering a malformed grid dns address, the viewer needs to catch the malformed address. Currently it throws an error in a subroutine of the grid-address-handling (llviewernetwork.cpp, function 'addGrid') and crashes without warning due to a thrown error ("throw LLInvalidGridName(grid);"). This fix adds code to: notifications.xml (a warning message text/notification: "IllegalGridName"), llviewernetwork.h (a function which checks whether a given grid exists, by name (string): bool getGridExists), llpanellogin.cpp (an include for llviewernetwork.h, and a check against an invalid grid dns address in function onSelectServer). Todo: Maybe edit the notification message, and update the language files. Testing Test plan: Insert a malformed grid dns address (e.g. http://grid.francogrid.org:8002 which works with some TPVs) into the grid selection combo on the login screen. Expected behaviour: a popup shows up, informing about the malformed address, and the grid selection combo swaps back to the default grid (Agni). Bugs: STORM-1818 Diffs