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

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

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Storage
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: cpeterson, Assigned: Akshay CV, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
mozilla51
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
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().
(Reporter)

Comment 2

a year ago
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");
(Assignee)

Comment 3

a year ago
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?
(Assignee)

Comment 4

a year ago
Created attachment 8783008 [details] [diff] [review]
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"
Attachment #8783008 - Flags: review?(cpeterson)
(Reporter)

Comment 5

a year ago
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
Attachment #8783008 - Flags: review?(cpeterson) → feedback+
(Reporter)

Updated

a year ago
Assignee: nobody → akshayharidas006
(Assignee)

Comment 6

a year ago
Created attachment 8783203 [details] [diff] [review]
Bug1296182-Replace_PR_Abort_with_MOZ_CRASH.diff

I replaced NSPR's PR_Abort() function with MFBT's MOZ_CRASH("SQLite Version Error") macro
Attachment #8783203 - Flags: review?(cpeterson)
(Reporter)

Comment 7

a year ago
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
Attachment #8783203 - Flags: review?(cpeterson) → feedback+
(Reporter)

Comment 8

a year ago
(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").
(Assignee)

Comment 9

a year ago
Created attachment 8783989 [details] [diff] [review]
Bug1296182_Replace_PR_Abort_with_MOZ_CRASH.diff

I replaced ::PR_ABORT with MOZ_CRASH("SQLite Version Error").
Attachment #8783989 - Flags: review?(cpeterson)
(Reporter)

Comment 10

a year ago
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
Attachment #8783989 - Flags: review?(cpeterson) → feedback+
Comment hidden (obsolete)
(Reporter)

Updated

a year ago
Attachment #8783203 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Blocks: 461344
(Reporter)

Comment 12

a year ago
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.
Attachment #8783989 - Flags: review?(bugmail)
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!
Attachment #8783989 - Flags: review?(bugmail) → review+

Comment 14

a year ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3490f9010729
Replace PR_Abort() with MOZ_CRASH("SQLite Version Error"). r=asuth
(Reporter)

Comment 15

a year ago
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
status-firefox51: --- → fixed

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3490f9010729
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 17

a year ago
okay,thank you :)
You need to log in before you can comment on or make changes to this bug.