Closed Bug 1296189 Opened 8 years ago Closed 8 years ago

Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH()

Categories

(Core :: General, defect, P3)

Product:
Core
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: cpeterson, Assigned: svezauzeto12, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(3 files)

patch.diff
93.01 KB, patch
cpeterson
: feedback+
Details | Diff | Splinter Review
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().
58 bytes, text/x-review-board-request
cpeterson
: feedback+
Details
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().
58 bytes, text/x-review-board-request
froydnj
: review+
Details 
On the dev-platform mailing list, Nick Nethercote suggested: "Can we just get rid of NS_RUNTIMEABORT? I count 308 occurrences of it in the code vs. 4064 for MOZ_CRASH."

https://groups.google.com/d/msg/mozilla.dev.platform/qMGMWepYK0A/gwg6hP9nPQAJ

The MOZ_CRASH() macro only accepts a string literal parameter, whereas NS_RUNTIMEABORT() accepts pointer variables. Therefore, only the following NS_RUNTIMEABORT() calls that pass a string literal message can be directly replaced with MOZ_CRASH():

http://searchfox.org/mozilla-central/search?q=%5CbNS_RUNTIMEABORT%5C%28%22&regexp=true&path=

The following NS_RUNTIMEABORT() calls that pass variable parameters *can't* easily be replaced with MOZ_CRASH. These calls do not need to be replaced to fix this bug.

http://searchfox.org/mozilla-central/search?q=%5CbNS_RUNTIMEABORT%5C(%5Ba-zA-Z%5D&case=false&regexp=true&path=
For what its worth, I fixed the PID issue njn mentioned in bug 1286005.
dbaron has some kind of unification work going on in bug 1277448, but that's a little more involved than just a replacement like you are talking about.
I think the only concern I have is this bit of code:

http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp#383

If the annotations that MOZ_CRASH adds are equivalent here and we don't care about losing the app note bit, then yes, let's do this.
Attached patch patch.diff — — Splinter Review 
So, I wrote a patch for this; however, this being my second-ish patch, I'm not sure how to test any of the it other than running |mach build| because some of the changes affect test cases themselves.
Attachment #8782679 - Flags: review?(cpeterson)
Comment on attachment 8782679 [details] [diff] [review]
patch.diff

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

Thanks, William! Your patch looks good. Here are some minor suggestions.

In the meantime, I will push your patch to the Try servers for testing.

::: dom/base/WebSocket.cpp
@@ +1989,1 @@
>        return NS_ERROR_UNEXPECTED;

Since MOZ_CRASH will never return, you can remove any `return` statements after MOZ_CRASH you add.

However, some compilers may incorrectly warn about functions not returning a value. In that case, you can leave the previous (unused) `return` statement to placate the compiler. :-) The rule-of-thumb is that you should leave the `return` where MOZ_CRASH would other become the last statement before the end of the function.

::: dom/devicestorage/DeviceStorageRequestChild.cpp
@@ +143,1 @@
>        break;

Similarly, you can remove any `break` statements after MOZ_CRASH calls you add.

::: dom/ipc/ContentChild.cpp
@@ +2057,1 @@
>    return nullptr;

These two `return nullptr;` statements are good examples of code that might cause "missing return" warnings if they were removed.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +874,1 @@
>    }

This is a very strange loop! Maybe replace this entire `for` loop with something like `MOZ_RELEASE_ASSERT(replies.Length() > 0, "should have no replies");`.

The loop was changed in https://hg.mozilla.org/mozilla-central/diff/1494a4e9ab4f/gfx/layers/ipc/ImageBridgeChild.cpp#l513.

::: layout/generic/nsFrame.cpp
@@ +5213,1 @@
>                      "document hierarchies?");

Please fix the indentation of the second line so the opening quotes are aligned.

::: layout/style/nsComputedDOMStyle.cpp
@@ +588,2 @@
>    // Just in case NS_RUNTIMEABORT ever stops killing us for some reason
>    aCSSParseEnv.mPrincipal = nullptr;

You can just remove the code and comments after the MOZ_CRASH because MOZ_CRASH will never return.

::: netwerk/ipc/NeckoMessageUtils.h
@@ +101,1 @@
>                        "https://bugzilla.mozilla.org/show_bug.cgi?id=661158");

Please fix the indentation of the second line of code so the opening quotes are aligned.
Attachment #8782679 - Flags: review?(cpeterson) → feedback+
Assignee: nobody → schmittw
William, are you using hg? You will need to add a commit summary to your patch that looks something like:

Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().
(In reply to Chris Peterson [:cpeterson] from comment #5)
> ::: dom/base/WebSocket.cpp
> @@ +1989,1 @@
> >        return NS_ERROR_UNEXPECTED;
> 
> Since MOZ_CRASH will never return, you can remove any `return` statements
> after MOZ_CRASH you add.
> 
> However, some compilers may incorrectly warn about functions not returning a
> value. In that case, you can leave the previous (unused) `return` statement
> to placate the compiler. :-) The rule-of-thumb is that you should leave the
> `return` where MOZ_CRASH would other become the last statement before the
> end of the function.

William, my suggestion to remove the now-unnecessary return statements is purely optional. If you don't have time to determine removals will cause new compiler warnings, you can skip that suggestion.

But please submit a revised patch that fixes the indentation and replaces the strange loop with MOZ_RELEASE_ASSERT. :-)
(In reply to Chris Peterson [:cpeterson] from comment #6)
> William, are you using hg? You will need to add a commit summary to your
> patch that looks something like:
> 
> Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with
> MOZ_CRASH().

This MDN page has more information about using hg to create a patch with a commit message and your name. The page warnings that "This document is outdated." but it is still useful if you are using hg patch queues (aka "mq"). Diving into hg's branching and bookmarks is a bigger time commitment, but enables a more git-like workflow, if that is what you prefer.

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial
Here is an MSVC compile error from the Try server build:

https://treeherder.mozilla.org/logviewer.html#?job_id=25986900&repo=try#L7195

> dom\ipc\contentchild.cpp(530) : warning C4722: 'mozilla::dom::ContentChild::~ContentChild':
> destructor never returns, potential memory leak 

Looks like MSVC doesn't like the ~ContentChild destructor calling a no-return function, even though we're doing so on purpose. MSVC didn't complain about NS_RUNTIMEABORT because it couldn't tell that NS_RUNTIMEABORT never returns.

We have two options here:

1. Suppress the warning using `#pragma warning` push and pop.
2. Revert the MOZ_CRASH back to NS_RUNTIMEABORT.

#2 is a reasonable option because this bug's goal was not aim to replace all calls to NS_RUNTIMEABORT, just those that can be directly replaced with MOZ_CRASH.

That said, I recommend option #1. It is ugly but someone will need to make this change eventually if we retire NS_RUNTIMEABORT everywhere. :) Here is an example of how to suppress MSVC's warnings:

  http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/js/src/vm/Runtime.h#53
  http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/js/src/vm/Runtime.h#1708
Here is an XPCShell test failure on Linux and Windows:

https://treeherder.mozilla.org/logviewer.html#?job_id=25988232&repo=try#L7592
https://treeherder.mozilla.org/logviewer.html#?job_id=25989942&repo=try#L15341

> TEST-UNEXPECTED-FAIL | toolkit/crashreporter/test/unit/test_crash_runtimeabort.js |
> run_test/< - [run_test/< : 15] false == true

This test purposely generates a crash dump. The test is failing because the crash dump is missing the "xpcom_runtime_abort" annotation:

http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/toolkit/crashreporter/test/unit/test_crash_runtimeabort.js#15

This is exactly the issue Nathan highlighted in comment 3 above. Nathan linked to the code that adds the "xpcom_runtime_abort" annotation here:

http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/xpcom/base/nsDebugImpl.cpp#390

To fix this test, I think you can just revert the MOZ_CRASH in toolkit/crashreporter/test/nsTestCrasher.cpp back to NS_RUNTIMEABORT. That change will preserve the existing semantics of CrashTestUtils.CRASH_RUNTIMEABORT, but points to an inconsistency in the MOZ_CRASH and NS_RUNTIMEABORT crash dumps' debug info. This is not a new problem because we already have thousands of calls to MOZ_CRASH.

I think adding support for annotations is bug 1277448 that Andrew mentioned in comment 2 above.
btw, this bug is a good example of how a "simple" search-and-replace change in a big project is never as easy as it first seems. There are lots of unknown side effects hiding under the rug. :)
Hi William, are you still working on this bug? Your patch looks good and only needs a couple minor fixes.
Flags: needinfo?(schmittw)
See Also: → 1277448
Hi William, are you still working on this bug?
If this is open I would like to try to fix it .
(In reply to svezauzeto12 from comment #15)
> If this is open I would like to try to fix it .

Hi, svezauzeto12! Have you compiled and run your own build of Firefox already? That's the first step. Here are the build instructions:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

If you have questions about compiling the Firefox code, just drop by the #introduction IRC channel on Mozilla's IRC server:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
I am building it right now, should be good to go in about hour.
I am ready to work on this.
Cool! William CS attached a work-in-progress patch to this bug in comment 4. You may want to download his patch and use it as a starting place. I had wrote some feedback in comment 10 and comment 11 that will need to be fixed in William's patch.

If you would rather start your own patch, then see my comment 0 for a description of the macro calls that will need to be find-and-replaced. You will also need to handle the special cases I noted in comment 10 and comment 11.

https://bug1296189.bmoattachments.org/attachment.cgi?id=8782679
Flags: needinfo?(schmittw)
Hope I made it right.
Comment on attachment 8813846 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/95180/#review95336

Thanks! Your pull request look great. There are just a few minor issues that need to be fixed before I can run your code on the Try test servers.

::: dom/ipc/ContentChild.cpp:187
(Diff revision 1)
>  #endif
>  
> +#ifdef _MSC_VER
> +#pragma warning(push)
> +#pragma warning(disable:4100) /* Silence unreferenced formal parameter warnings */
> +#endif

Why is this #pragma to silence MSVC warning 4100 (unreferenced parameter warning) necessary? 

Was this #pragma to address the "destructor never returns, potential memory leak" warning I mentioned in comment 10? https://bugzilla.mozilla.org/show_bug.cgi?id=1296189#c10

That was MSVC warning 4722, not 4100. I think we will need a #pragma to disable 4722, but we should place the #pragma directly before the ~ContentChild destructor. We will also want to re-enable 4722 after the destructor. This will minimize the amount of code with the warning disabled.

::: gfx/thebes/gfxPlatform.cpp:2462
(Diff revision 1)
>        NS_WARNING("OpenGL-accelerated layers are not supported on this system");
>        tell_me_once = 1;
>      }
>  #ifdef MOZ_WIDGET_ANDROID
> -    NS_RUNTIMEABORT("OpenGL-accelerated layers are a hard requirement on this platform. "
> +    MOZ_CRASH("OpenGL-accelerated layers are a hard requirement on this platform. "
>                      "Cannot continue without support for them");

Please fix the indentation of the second line so the left quotes are aligned.

::: layout/generic/nsFrame.cpp:5829
(Diff revision 1)
>                 "hierarchies?");
>    if (PresContext()->GetRootPresContext() !=
>          aOther->PresContext()->GetRootPresContext()) {
>      // crash right away, we are almost certainly going to crash anyway.
> -    NS_RUNTIMEABORT("trying to get the offset between frames in different "
> +    MOZ_CRASH("trying to get the offset between frames in different "
>                      "document hierarchies?");

Please fix the indentation of the second line so the left quotes are aligned.

::: toolkit/crashreporter/test/nsTestCrasher.cpp:48
(Diff revision 1)
>  // Keep these in sync with CrashTestUtils.jsm!
>  const int16_t CRASH_INVALID_POINTER_DEREF = 0;
>  const int16_t CRASH_PURE_VIRTUAL_CALL     = 1;
>  const int16_t CRASH_RUNTIMEABORT          = 2;
>  const int16_t CRASH_OOM                   = 3;
> -const int16_t CRASH_MOZ_CRASH             = 4;
> +const int16_t CRASH_NS_RUNTIMEABORT             = 4;

Please fix the indentation of this line so the equal signs are aligned.

::: toolkit/crashreporter/test/nsTestCrasher.cpp:77
(Diff revision 1)
>      mozilla::Unused << moz_xmalloc((size_t) -1);
>      mozilla::Unused << moz_xmalloc((size_t) -1);
>      break;
>    }
> -  case CRASH_MOZ_CRASH: {
> -    MOZ_CRASH();
> +  case CRASH_NS_RUNTIMEABORT: {
> +    NS_RUNTIMEABORT();

Unlike the MOZ_CRASH() macro, NS_RUNTIMEABORT() requires a message parameter. I suggest something like for this test:

NS_RUNTIMEABORT("CRASH_NS_RUNTIMEABORT");
Sorry about that pragma warning. This is the first time I am working with it and I may have been bit too hasteful process of working on this bug.

I may have failed pragma warning also this time, but I hope it will work.
Comment on attachment 8813846 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/95180/#review95384

::: dom/ipc/ContentChild.cpp:513
(Diff revisions 1 - 2)
>    // multiprocess mode!
>    nsDebugImpl::SetMultiprocessMode("Child");
>  }
>  
> +
> +#pragma warning(disable:4100) /* Silence unreferenced formal parameter warnings */

This is the right location for these #pragmas. We should also:

1. Wrap these #pragmas in #ifdef _MSC_VER because they are only understood by MSVC.
2. Disable warning 4722 instead of 4100.
3. Use #pragma warning push/disable/pop instead of push/default.

Like this example code:

#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable:4722) /* Silence "destructor never returns" warning */
#endif

...

#ifdef _MSC_VER
#pragma warning(pop)
#endif
Don't forget the indentation fixes for these quoted strings. The left quotes on each line should be aligned.

> ::: gfx/thebes/gfxPlatform.cpp:2462
> > -    NS_RUNTIMEABORT("OpenGL-accelerated layers are a hard requirement on this platform. "
> > +    MOZ_CRASH("OpenGL-accelerated layers are a hard requirement on this platform. "
> >                      "Cannot continue without support for them");
                   ^-----^

> ::: layout/generic/nsFrame.cpp:5829
> > -    NS_RUNTIMEABORT("trying to get the offset between frames in different "
> > +    MOZ_CRASH("trying to get the offset between frames in different "
> >                      "document hierarchies?");
                   ^-----^
Assignee: schmittw → svezauzeto12
Status: NEW → ASSIGNED
Hi, I have made changes you wrote above, I am just not sure did I push right commit since I am still learning how to use mercurial.

If it not good I will do it again.

Cheers
Comment on attachment 8813846 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/95180/#review95890

Looks good! You've fixed all the minor code issues I noted. Just a few more items. Thanks for your patience! :)

1. Your commits do not apply cleanly to the latest mozilla-inbound repo. There were some merge conflicts in the gfx/layers directory and some test files in storage/test have moved to storage/test/gtest. You will need to rebase your commit to the latest mozilla-central or mozilla-inbound.
2. If you have made multiple Mercurial commits to address my review feedback, I think you will need to squash all your commits into one good commit.
3. I have pushed your patch to the Try test server to make sure it builds on all platforms. If the tests look good, then I will find a final code reviewer to sign off on your changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=becd366670a00bdc86990043a7c69e2d781b5f4e
I will make hg update --clean central and then just squash my changes into it , is that acceptable ?

I think I already squashed all my commits into latest one.

Thanks
Please check it now.
(In reply to Chris Peterson [:cpeterson] from comment #31)
> 3. I have pushed your patch to the Try test server to make sure it builds on
> all platforms. If the tests look good, then I will find a final code
> reviewer to sign off on your changes.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=becd366670a00bdc86990043a7c69e2d781b5f4e

The Try test builds look good. The "Linux x64 opt" test failures are an unrelated issue where the test machine has an old version of zlib (bug 910477).
Attachment #8813846 - Flags: review?(nfroyd)
@ Nathan, Tomislav's patch replaces NS_RUNTIMEABORT calls that have static string parameters with MOZ_CRASH. The changes span many directories. Since NS_RUNTIMEABORT is an XPCOM thing, do you feel comfortable signing off on all these changes? Or should Tomislav split out the changes for each directory for respective module reviewers?

@ Tomislav, one last thing: you will need to update your commit message. It is currently "Bug 1296189 - Downloaded patch and fixed it up according to comments #10 and #11; r?cpeterson" but you will want to change it to something like "Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH()."
Flags: needinfo?(nfroyd)
Okay, should I add "r?cpeterson" to commit message also or should I leave it out ?
(In reply to Tomislav Jurin from comment #37)
> Okay, should I add "r?cpeterson" to commit message also or should I leave it
> out ?

I think you can leave out "r?username" and IIUC MozReview's Autoland will add it when the final reviewer clicks the "r+" button. I won't be the final reviewer.
I made commit before I read your comment, will just push again, sorry.
Attachment #8813846 - Flags: review?(nfroyd)
(In reply to Chris Peterson [:cpeterson] from comment #36)
> @ Nathan, Tomislav's patch replaces NS_RUNTIMEABORT calls that have static
> string parameters with MOZ_CRASH. The changes span many directories. Since
> NS_RUNTIMEABORT is an XPCOM thing, do you feel comfortable signing off on
> all these changes? Or should Tomislav split out the changes for each
> directory for respective module reviewers?

I can look at this patch today.
Flags: needinfo?(nfroyd)
Comment on attachment 8813846 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/95180/#review96800

::: toolkit/crashreporter/test/nsTestCrasher.cpp:48
(Diff revision 7)
>  // Keep these in sync with CrashTestUtils.jsm!
>  const int16_t CRASH_INVALID_POINTER_DEREF = 0;
>  const int16_t CRASH_PURE_VIRTUAL_CALL     = 1;
>  const int16_t CRASH_RUNTIMEABORT          = 2;
>  const int16_t CRASH_OOM                   = 3;
> -const int16_t CRASH_MOZ_CRASH             = 4;
> +const int16_t CRASH_NS_RUNTIMEABORT       = 4;

This change looks like it is going the opposite direction of what we want to do; `CRASH_RUNTIMEABORT` just above already crashes via `NS_RUNTIMEABORT`.  Please revert.

::: toolkit/crashreporter/test/nsTestCrasher.cpp:76
(Diff revision 7)
> -  case CRASH_MOZ_CRASH: {
> -    MOZ_CRASH();
> +  case CRASH_NS_RUNTIMEABORT: {
> +    NS_RUNTIMEABORT("CRASH_NS_RUNTIMEABORT");
>      break;

Likewise here.  Please revert.
Attachment #8813846 - Flags: review?(nfroyd)
Comment on attachment 8813846 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/95180/#review96800

Sorry, clicked the wrong button!  This looks good except for the two minor issues I've noted.  Please fix those, and then we can test everything to ensure that the patch is working as intended.
Attachment #8813846 - Flags: review?(cpeterson) → feedback+
If I understand correctly what you wanted this should be it now.

Thanks
I don't know why I have so much differences in these commits, at the time I submited them "hg diff" only showed what I wanted to commit (reverting back what you said), now everything got dangled up.
+ it also changed my commit message
This latest should have the changes you wanted, if I am not mistaken.

Thanks to guys from #mercurial
(In reply to Tomislav Jurin from comment #49)
> I don't know why I have so much differences in these commits, at the time I
> submited them "hg diff" only showed what I wanted to commit (reverting back
> what you said), now everything got dangled up.

I think MozReview is confused because you rebased your changes on top of a newer mozilla-central revision (as I had suggested) than your original review request. Sorry for the confusion. I don't really understand MozReview's workflow.

AFAICT, your nsTestCrasher.cpp change looks good. Your latest file revision matches the original.

I started another Try test run. If that looks good, I can manually land your patch and fix up any merge conflicts from the mozilla-central rebase.
(In reply to Chris Peterson [:cpeterson] from comment #54)
> (In reply to Tomislav Jurin from comment #49)
> > I don't know why I have so much differences in these commits, at the time I
> > submited them "hg diff" only showed what I wanted to commit (reverting back
> > what you said), now everything got dangled up.
> 
> I think MozReview is confused because you rebased your changes on top of a
> newer mozilla-central revision (as I had suggested) than your original
> review request. Sorry for the confusion. I don't really understand
> MozReview's workflow.
> 
> AFAICT, your nsTestCrasher.cpp change looks good. Your latest file revision
> matches the original.
> 
> I started another Try test run. If that looks good, I can manually land your
> patch and fix up any merge conflicts from the mozilla-central rebase.

That would be great,

Thank you.
(In reply to Nathan Froyd [:froydnj] from comment #44)
> Sorry, clicked the wrong button!  This looks good except for the two minor
> issues I've noted.  Please fix those, and then we can test everything to
> ensure that the patch is working as intended.

Nathan, would you like to re-review Tomislav's patch? He has posted a new patch that reverts the unwanted CRASH_NS_RUNTIMEABORT changes in nsTestCrasher.cpp.

https://reviewboard.mozilla.org/r/95180/diff/10#index_header

https://hg.mozilla.org/try/rev/cc57a9633596cec4b7d7c108240dcb160f636d69

> I started another Try test run. If that looks good, I can manually land your
> patch and fix up any merge conflicts from the mozilla-central rebase.

The new Try test run looks good. There were a few test failures, but they are unrelated intermittent problems.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb7b2b96b54a
Flags: needinfo?(nfroyd)
Comment on attachment 8815872 [details]
Bug 1296189 - Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH().

https://reviewboard.mozilla.org/r/96654/#review97540

This change looks great, thank you!
Attachment #8815872 - Flags: review+
(In reply to Chris Peterson [:cpeterson] from comment #56)
> (In reply to Nathan Froyd [:froydnj] from comment #44)
> > Sorry, clicked the wrong button!  This looks good except for the two minor
> > issues I've noted.  Please fix those, and then we can test everything to
> > ensure that the patch is working as intended.
> 
> Nathan, would you like to re-review Tomislav's patch? He has posted a new
> patch that reverts the unwanted CRASH_NS_RUNTIMEABORT changes in
> nsTestCrasher.cpp.

Done.  If you could take care of munging the two patches together and landing them, that would be great.  Thanks!
Flags: needinfo?(nfroyd)
Thanks. I'll merge the two patches and land them on behalf of Tomislav soon.
Attachment #8813846 - Flags: review?(cpeterson) → feedback+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afe43384706c
Replace NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH(). r=froydnj
Tomislav, I landed your fixes on mozilla-inbound and everything looks good. Thanks for your contribution and patience with so many review rounds! :) This bug required changes to more files than most bugs, so this should not be the typical review experience.
https://hg.mozilla.org/mozilla-central/rev/afe43384706c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
See Also: → 1368274
Is it expected that there are still instances of NS_RUNTIMEABORT(<string literal>) in the tree?

https://dxr.mozilla.org/mozilla-central/search?q=NS_RUNTIMEABORT
Flags: needinfo?(cpeterson)
(In reply to David Major [:dmajor] from comment #63)
> Is it expected that there are still instances of NS_RUNTIMEABORT(<string
> literal>) in the tree?

Nope! Some of those calls were added after this bug was resolved, but I guess we missed some others.

@ froydnj: is there any reason to prefer NS_RUNTIMEABORT() over MOZ_CRASH() or MOZ_CRASH_UNSAFE_OOL()?

I see 50 instances of NS_RUNTIMEABORT(<string literal>) and 25 instances of NS_RUNTIMEABORT(variable). I think they all could be replaced by MOZ_CRASH(<string literal>) and MOZ_CRASH_UNSAFE_OOL(variable), respectively. We could then remove NS_RUNTIMEABORT() and supporting machinery in NS_DebugBreak().

https://searchfox.org/mozilla-central/search?q=NS_RUNTIMEABORT(%22&case=true

https://searchfox.org/mozilla-central/search?q=NS_RUNTIMEABORT%5C(%5Cw&case=true&regexp=true
Flags: needinfo?(cpeterson)
Flags: needinfo?(nfroyd)
(In reply to Chris Peterson [:cpeterson] from comment #64)
> (In reply to David Major [:dmajor] from comment #63)
> > Is it expected that there are still instances of NS_RUNTIMEABORT(<string
> > literal>) in the tree?
> 
> Nope! Some of those calls were added after this bug was resolved, but I
> guess we missed some others.
> 
> @ froydnj: is there any reason to prefer NS_RUNTIMEABORT() over MOZ_CRASH()
> or MOZ_CRASH_UNSAFE_OOL()?

The reasons in comment 3 still apply, but it's not clear that we care about the app notes anymore, since we have Assertions.h:gMozCrashReason, which the crashreporter understands.

I think we can use MOZ_CRASH for NS_RUNTIMEABORT(<string literal>) without question.  Per the comment in Assertions.h, MOZ_CRASH_UNSAFE_OOL usages will require data review.
Flags: needinfo?(nfroyd)
Perhaps some cpp-fu could make NS_RUNTIMEABORT error on char(&)[].
Blocks: 1412048
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: