1296182 - Replace PR_Abort() with MOZ_CRASH() in storage/mozStorageService.cpp

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P5)

Description

[Chris Peterson [:cpeterson]]

We want to replace NSPR's PR_Abort() function with MFBT's MOZ_CRASH() macro.

According to the following code search, there is only one remaining use of PR_Abort in Mozilla's C++ code. C code in the nsprpub/ directories may use PR_MAX and should not be changed.

http://searchfox.org/mozilla-central/search?q=symbol:_Z8PR_Abort&redirect=false

storage/mozStorageService.cpp
231	::PR_Abort(); // found in mozilla::storage::Service::getSingleton

Comment 1

[Chris Peterson [:cpeterson]]

We can also remove mozStorageService.cpp's #include "prinit.h" because we won't need this header file when we're no longer calling PR_Abort().

Comment 2

[Chris Peterson [:cpeterson]]

Because MOZ_CRASH() accepts an optional error message argument (unlike PR_Abort), we should replace this PR_Abort() with code that looks something like:

    MOZ_CRASH("SQLite Version Error");

Comment 3

[Akshay CV]

Hi Chris Peterson,I replaced NSPR's PR_Abort() function with MFBT's MOZ_CRASH() macro.I am attaching my patch below.
Can you please review it?

Comment 4

[Akshay CV]

Attachment: Bug 1296182 - Replaced PR_Abort() with MOZ_CRASH(""SQLite Version Error"")

Bug 1296182 - Replaced PR_Abort() with MOZ_CRASH(""SQLite Version Error"")in storage/mozStorageService.cpp"

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8783008 [details] [diff] [review]
Bug 1296182 - Replaced PR_Abort() with MOZ_CRASH(""SQLite Version Error"")

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

Thanks, Akshay! Your code looks good. There is just one minor issue with your patch.

::: storage/mozStorageService.cpp
@@ -226,4 @@
>                            "run.");
>        (void)ps->Alert(nullptr, title.get(), message.get());
>      }
> -    MOZ_CRASH();

The original code is ::PR_Abort(). I suspect you have an earlier patch that changed PR_Abort() to MOZ_CRASH(), followed by this patch changing MOZ_CRASH() to MOZ_CRASH("SQLite Version Error"). These two patches should be squashed into one patch.

If you are using hg's patch queue ("mq"), you can use the "hg qfold" command to "fold" the two patches into one. Here are hg instructions:

  https://www.mercurial-scm.org/wiki/EditingHistory#Editing_recent_history_with_MQ

If you are using git, then you can use "git rebase". Here are git instructions:

  http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Comment 6

[Akshay CV]

Attachment: Bug1296182-Replace_PR_Abort_with_MOZ_CRASH.diff

I replaced NSPR's PR_Abort() function with MFBT's MOZ_CRASH("SQLite Version Error") macro

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 8783203 [details] [diff] [review]
Bug1296182-Replace_PR_Abort_with_MOZ_CRASH.diff

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

::: storage/mozStorageService.cpp
@@ -955,5 @@
>        getConnections(connections);
>        for (uint32_t i = 0, n = connections.Length(); i < n; i++) {
>          if (!connections[i]->isClosed()) {
> -          
> -::PR_Abort();

The version of mozStorageService.cpp I see in mozilla-central calls MOZ_CRASH() on line 959, not PR_Abort(). Maye you had some changes in your local file before you created this patch?

https://hg.mozilla.org/mozilla-central/file/b09d90288666/storage/mozStorageService.cpp#l959

The only instance of PR_Abort() is on line 231. That's the line of code that should be changed to MOZ_CRASH("SQLite Version Error").

https://hg.mozilla.org/mozilla-central/file/b09d90288666/storage/mozStorageService.cpp#l231

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #7)
> The only instance of PR_Abort() is on line 231. That's the line of code that
> should be changed to MOZ_CRASH("SQLite Version Error").
> 
> https://hg.mozilla.org/mozilla-central/file/b09d90288666/storage/
> mozStorageService.cpp#l231

Your first patch (in comment 2) changed the correct line of code. The only problem with the first patch was that its "base" revision was not the original file with PR_Abort(). I think you had changed PR_Abort() to MOZ_CRASH() and then committed that code change before making a second code change from MOZ_CRASH() to MOZ_CRASH("SQLite Version Error"). You just need to combine those two changes into one patch that changes PR_Abort() directly to MOZ_CRASH("SQLite Version Error").

Comment 9

[Akshay CV]

Attachment: Bug1296182_Replace_PR_Abort_with_MOZ_CRASH.diff

I replaced ::PR_ABORT with MOZ_CRASH("SQLite Version Error").

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 8783989 [details] [diff] [review]
Bug1296182_Replace_PR_Abort_with_MOZ_CRASH.diff

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

Your patch looks great to me. Thanks for noticing that we can now remove prinit.h.

I started a Try build with your patch. When the Try build passes, I will ask someone for a final code review and then we can land your code.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec0868c034e2

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 8783008 [details] [diff] [review]
Bug 1296182 - Replaced PR_Abort() with MOZ_CRASH(""SQLite Version Error"")

># HG changeset patch
># Parent  1307527df12f94a7827dc1a131a49aa0326f62cd
>Bug 1296182 - Replaced PR_Abort() with MOZ_CRASH()in storage/mozStorageService.cpp
>
>diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp
>--- a/storage/mozStorageService.cpp
>+++ b/storage/mozStorageService.cpp
>@@ -226,7 +226,7 @@ Service::getSingleton()
>                           "run.");
>       (void)ps->Alert(nullptr, title.get(), message.get());
>     }
>-    MOZ_CRASH();
>+    MOZ_CRASH("SQLite Version Error");
>   }
> 
>   // The first reference to the storage service must be obtained on the

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8783989 [details] [diff] [review]
Bug1296182_Replace_PR_Abort_with_MOZ_CRASH.diff

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

Andrew, can you please review Akshay's patch? He has removed the last use of NSPR's PR_Abort() function outside of nsprpub, replacing it with MFBT's MOZ_CRASH and an error message. His patch LGTM and builds successfully on Try.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8783989 [details] [diff] [review]
Bug1296182_Replace_PR_Abort_with_MOZ_CRASH.diff

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

Yes, looks great to me too.  Thanks very much for the patch!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3490f9010729
Replace PR_Abort() with MOZ_CRASH("SQLite Version Error"). r=asuth

Comment 15

[Chris Peterson [:cpeterson]]

Thanks for your contribution, Akshay! If you're looking for another bug to work on, check out the list of bugs on the Bugs Ahoy site:

http://www.joshmatthews.net/bugsahoy/?js=1&cpp=1&unowned=1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3490f9010729

Comment 17

[Akshay CV]

okay,thank you :)