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

RESOLVED FIXED in Firefox 53

Status

()

Core
General
P3
normal
RESOLVED FIXED
10 months ago
a month ago

People

(Reporter: cpeterson, Assigned: Tomislav Jurin, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [lang=c++])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

10 months ago
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.

Comment 3

10 months ago
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.

Comment 4

10 months ago
Created attachment 8782679 [details] [diff] [review]
patch.diff

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)
(Reporter)

Comment 5

10 months ago
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+
(Reporter)

Updated

10 months ago
Assignee: nobody → schmittw
(Reporter)

Comment 6

10 months ago
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().
(Reporter)

Comment 7

10 months ago
(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. :-)
(Reporter)

Comment 8

10 months ago
Try server push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=697ea1e0c992
(Reporter)

Comment 9

10 months ago
(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
(Reporter)

Comment 10

10 months ago
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
(Reporter)

Comment 11

10 months ago
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.
(Reporter)

Comment 12

10 months ago
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. :)
(Reporter)

Comment 13

10 months ago
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: → bug 1277448
Hi William, are you still working on this bug?
(Assignee)

Comment 15

7 months ago
If this is open I would like to try to fix it .
(Reporter)

Comment 16

7 months ago
(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
(Assignee)

Comment 17

7 months ago
I am building it right now, should be good to go in about hour.
(Assignee)

Comment 18

7 months ago
I am ready to work on this.
Comment hidden (typo)
(Reporter)

Comment 20

7 months ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

7 months ago
Hope I made it right.
(Reporter)

Comment 23

7 months ago
mozreview-review
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");
(Assignee)

Comment 24

7 months ago
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 hidden (mozreview-request)
(Reporter)

Comment 26

7 months ago
mozreview-review
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
(Reporter)

Comment 27

7 months ago
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
(Assignee)

Comment 28

7 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 31

7 months ago
mozreview-review
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
(Assignee)

Comment 32

7 months ago
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
(Assignee)

Comment 33

7 months ago
Please check it now.
Comment hidden (mozreview-request)
(Reporter)

Comment 35

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

Updated

7 months ago
Attachment #8813846 - Flags: review?(nfroyd)
(Reporter)

Comment 36

7 months ago
@ 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)
(Assignee)

Comment 37

7 months ago
Okay, should I add "r?cpeterson" to commit message also or should I leave it out ?
(Reporter)

Comment 38

7 months ago
(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.
Comment hidden (mozreview-request)
(Assignee)

Comment 40

7 months ago
I made commit before I read your comment, will just push again, sorry.
Comment hidden (mozreview-request)
(Reporter)

Updated

7 months ago
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 43

7 months ago
mozreview-review
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 44

7 months ago
mozreview-review-reply
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.
(Reporter)

Updated

7 months ago
Attachment #8813846 - Flags: review?(cpeterson) → feedback+
(Assignee)

Comment 45

7 months ago
If I understand correctly what you wanted this should be it now.

Thanks
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 49

7 months ago
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.
(Assignee)

Comment 50

7 months ago
+ it also changed my commit message
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 53

7 months ago
This latest should have the changes you wanted, if I am not mistaken.

Thanks to guys from #mercurial
(Reporter)

Comment 54

7 months ago
(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.
(Assignee)

Comment 55

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

Comment 56

7 months ago
(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 57

7 months ago
mozreview-review
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)
(Reporter)

Comment 59

7 months ago
Thanks. I'll merge the two patches and land them on behalf of Tomislav soon.
(Reporter)

Updated

7 months ago
Attachment #8813846 - Flags: review?(cpeterson) → feedback+

Comment 60

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

Comment 61

7 months ago
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.

Comment 62

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afe43384706c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a month ago
See Also: → bug 1368274
You need to log in before you can comment on or make changes to this bug.