remove NS_ASSERT / debug.js

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: florian, Assigned: manikishanghantasala, Mentored)

Tracking

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

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

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [good-first-bug][lang=js][fxsearch])

Attachments

(1 attachment, 10 obsolete attachments)

31.24 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Filing in Places because NS_ASSERT is used mostly in Places code (21 callers).

https://searchfox.org/mozilla-central/search?q=NS_ASSERT&case=false&regexp=false&path=.js
Priority: -- → P3
Whiteboard: [fxsearch]
Mentor: mak77
Keywords: good-first-bug
Whiteboard: [fxsearch] → [good-first-bug][lang=js][fxsearch]
(Assignee)

Comment 1

a year ago
Hi, I would like to do this. Can you assign it to me?
Hi, we assign bugs when the first patch is attached. You are clearly welcome to work on it.

The scope here is to replace the NS_ASSERT calls in the above search (excluded the calls in httpd.js, since it has its own implementation) with appropriate:
if (some_condition)
  throw new Error("An appropriate error message");

Then remove the following imports
https://searchfox.org/mozilla-central/search?q=%22resource%3A%2F%2Fgre%2Fmodules%2Fdebug.js%22&case=false&regexp=false&path=

Finally remove the debug.js file itself, and its references from
toolkit/modules/moz.build
tools/lint/eslint/modules.json
(Assignee)

Comment 3

a year ago
Ok, Thanks.I will do that.
(Assignee)

Comment 4

a year ago
Hi,
I have done the changes in browser/components/feeds/FeedConverter.js If it is the right way to proceed .I will continue
Attachment #8944158 - Flags: review?(florian)
(Assignee)

Comment 5

a year ago
Posted patch bug-1431050-v2.patch (obsolete) — Splinter Review
Attachment #8944237 - Flags: review?(mak77)
Florian, do we have a replacement for it? Personally I think we should be using assertions more in front-end code and it was something I have been meaning to bring up as a discussion topic.
Flags: needinfo?(florian)
Assignee: nobody → manikishanghantasala
Status: NEW → ASSIGNED
Comment on attachment 8944158 [details] [diff] [review]
Bug-1431050-v1.patch "I have done the changes in browser/components/feeds/FeedConverter.js"

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

::: browser/components/feeds/FeedConverter.js
@@ +395,4 @@
>     * See nsIFeedResultService.idl
>     */
>    addFeedResult(feedResult) {
> +    if(feedResult.uri != null){

note that NS_ASSERT is an assert, thus it activates if what it is asserting is false. You should invert the conditions  for throwing.
NS_ASSERT(feedResult.uri != null, "null URI!"); means "Check that feedResult.uri is not null, otherwise notify about the problem".
"NS_ASSERT(condition, msg)" is like "if (!condition) throw new Error(msg);"

@@ +396,5 @@
>     */
>    addFeedResult(feedResult) {
> +    if(feedResult.uri != null){
> +    throw new Error("null URI!");
> +    }

Please pay attention to the coding style. you can use eslint to check for a part of those, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint

For example here you you add a space after "if" and before "{", and properly indent the body.
Attachment #8944158 - Flags: review?(florian) → feedback-
Comment on attachment 8944237 [details] [diff] [review]
bug-1431050-v2.patch

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

please always amend (merge) your changes to create a patch that can be applied on top of a source tree head. You can use the Mercurial Evolve extension to hg commit --amend your changes and create a single patch. Other kind of diff patches may be hard to review and apply.
Attachment #8944237 - Flags: review?(mak77)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> Florian, do we have a replacement for it? Personally I think we should be
> using assertions more in front-end code and it was something I have been
> meaning to bring up as a discussion topic.

We don't have a replacement afaik.
Though the current very low usage and the fact in the past this has been REALLY annoying for Nightly users (we had cases where Nightly was unusable because of assert dialogs looping) makes it a good target for removal.

I totally agree it would be nice to have a replacement that doesn't just post to the console, but also somehow notifies us about the problem, but I don't think that's a show-stopper for removing something that clearly is lacking, while we wait for that replacement idea.
(Assignee)

Comment 10

a year ago
(In reply to Marco Bonardo [::mak] from comment #8)
> Comment on attachment 8944237 [details] [diff] [review]
> bug-1431050-v2.patch
> 
> Review of attachment 8944237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please always amend (merge) your changes to create a patch that can be
> applied on top of a source tree head. You can use the Mercurial Evolve
> extension to hg commit --amend your changes and create a single patch. Other
> kind of diff patches may be hard to review and apply.
Yes I will do. Thanks for saying to merge should I use "hg merge"?
I should have used the term "amend" or "fold", rather than "merge", I was trying to make it clearer, but likely made it worse.

Usually you use "hg amend" or "hg commit --amend" to commit pending changes to an existing changeset that you already committed.
If you have multiple changesets committed you can also fold them using the histedit extension and the fold option. The important thing is that they are only locally and not remotely committed (thus they are in the "draft" state).

Alternatively you can also use Mercurial Queues, and then you can use "hg qref" or "hg qfold" to do the same to your queue. This is an old way of working so I think you should rather try to learn the "new" way using amend.

hg merge is a different command, it is used when a changeset doesn't apply cleanly (because of underlying changes) to a tree, and then you need to merge your changest on top of the current tree. That's not what you are looking for.

In general, this documentation may help (Especially the workflows section)
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html
(Reporter)

Comment 12

a year ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> Florian, do we have a replacement for it? Personally I think we should be
> using assertions more in front-end code and it was something I have been
> meaning to bring up as a discussion topic.

I think the first step toward making assertions in the front-end code useful is to actually fail tests for them. Currently our tests still pass if the error console is full of errors. I think bug 1426219 is a major step in the right direction to solve this.

I don't think wanting good assertions in the future is a reason to hold on removing something that has very little use and is ugly (I'm pretty sure we would not want to call new front-end assertions "NS_ASSERT").
Flags: needinfo?(florian)
(Assignee)

Comment 13

a year ago
(In reply to Marco Bonardo [::mak] from comment #11)
> I should have used the term "amend" or "fold", rather than "merge", I was
> trying to make it clearer, but likely made it worse.
> 
> Usually you use "hg amend" or "hg commit --amend" to commit pending changes
> to an existing changeset that you already committed.
> If you have multiple changesets committed you can also fold them using the
> histedit extension and the fold option. The important thing is that they are
> only locally and not remotely committed (thus they are in the "draft" state).
> 
> Alternatively you can also use Mercurial Queues, and then you can use "hg
> qref" or "hg qfold" to do the same to your queue. This is an old way of
> working so I think you should rather try to learn the "new" way using amend.
> 
> hg merge is a different command, it is used when a changeset doesn't apply
> cleanly (because of underlying changes) to a tree, and then you need to
> merge your changest on top of the current tree. That's not what you are
> looking for.
> 
> In general, this documentation may help (Especially the workflows section)
> https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/
> index.html

Thanks for your reply from my next patch, I will do follow coding style in the file and do send the patch in the way you mentioned .
(Assignee)

Comment 14

a year ago
(In reply to Marco Bonardo [::mak] from comment #11)
> I should have used the term "amend" or "fold", rather than "merge", I was
> trying to make it clearer, but likely made it worse.
> 
> Usually you use "hg amend" or "hg commit --amend" to commit pending changes
> to an existing changeset that you already committed.
> If you have multiple changesets committed you can also fold them using the
> histedit extension and the fold option. The important thing is that they are
> only locally and not remotely committed (thus they are in the "draft" state).
> 
> Alternatively you can also use Mercurial Queues, and then you can use "hg
> qref" or "hg qfold" to do the same to your queue. This is an old way of
> working so I think you should rather try to learn the "new" way using amend.
> 
> hg merge is a different command, it is used when a changeset doesn't apply
> cleanly (because of underlying changes) to a tree, and then you need to
> merge your changest on top of the current tree. That's not what you are
> looking for.
> 
> In general, this documentation may help (Especially the workflows section)
> https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/
> index.html

Hi, In order to check whether my fix is working, Is there any other way than Building(./mach build)?
you can use an artifact build that will produce a build in a much faster way with ./mach build (you can get a full build in 5 minutes or less)
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Depending on the module where you removed the NS_ASSERT, you should be able to run some of the tests, usually xpcshell tests may be enough for this case (it really depends on the module): ./mach xpcshell-test path_to_the_module

We will then also run the patch on the Try Server that can run all the tests.
(Assignee)

Comment 16

a year ago
Thanks for the help
(Assignee)

Comment 17

a year ago
Hi, 
While replacing NS_ASSERT ,there is a NS_ASSERT call in 
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#14
Can anyone say if I should replace that too?
yes, comment 0 search also includes that.
That's not actually a call, it's an import (unused)
(Assignee)

Comment 19

a year ago
As it isn't a call can I remove it or is there any place where should I delete it's reference?
You can remove it. It's importing the NS_ASSERT symbol, but nothing uses it.
(Assignee)

Comment 21

a year ago
Thanks, I have something other to ask can I know in which cases I can use artifact builds?
(In reply to manikishanghantasala from comment #21)
> Thanks, I have something other to ask can I know in which cases I can use
> artifact builds?

See the "Restrictions" section at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds
(Assignee)

Comment 23

a year ago
Posted patch Bug-1431050-v3.patch (obsolete) — Splinter Review
Attachment #8946621 - Flags: review?(mak77)
Attachment #8944158 - Attachment is obsolete: true
Attachment #8944237 - Attachment is obsolete: true
Comment on attachment 8946621 [details] [diff] [review]
Bug-1431050-v3.patch

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

Just a hint: when attaching a new patch, you can mark previous ones as obsolete in the same form.

You should also hg rm toolkit/modules/debug.js

::: browser/components/feeds/FeedConverter.js
@@ +395,5 @@
>     * See nsIFeedResultService.idl
>     */
>    addFeedResult(feedResult) {
> +
> +    if (feedResult.uri != null)

The conditions in FeedConverter.js are still wrong and inverted, as percomment 7. Likely you started from a tree that had your previous patch applied, if you fold the changes you'll see better what I mean from the diff.

@@ +396,5 @@
>     */
>    addFeedResult(feedResult) {
> +
> +    if (feedResult.uri != null)
> +      throw new Error("null URI!");

This patch applies on top of the previous ones, please fold/squash them, we need a single patch that applies on top of mozilla-central.
Depending on the method you used, you can either hg qfold (if you used mercurial queues) or use hg histedit to "fold" draft changesets.

For the future, I'd suggest to use hg commit --amend to apply changes on top of a committed (but draft) changeset.
This has some documentation https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

::: browser/components/places/content/controller.js
@@ +677,3 @@
>     * receive a string instead of an nsIURI object.
>     */
>    _assertURINotString: function PC__assertURINotString(value) {

You can completely remove _assertURINotString and its javadoc, since it's apparently not used anywhere (https://searchfox.org/mozilla-central/search?q=_assertURINotString&case=false&regexp=false&path=)

@@ +1019,4 @@
>        let query = aContainerNode.getQueries()[0];
>        let beginTime = query.beginTime;
>        let endTime = query.endTime;
> +      if (!(query && beginTime && endTime))

This could be more readable:
if (!query || !beginTime || !endTime)

@@ +1039,4 @@
>      if (!this._hasRemovableSelection())
>        return;
>  
> +    if (aTxnName == undefined)

You can remove this check, remove aTxnName completely, and correct _removeRowsFromBookmarks to take no argument, since the current argument (txnname) is unused!

::: browser/components/places/content/places.js
@@ +974,3 @@
>      if (startID) {
>        var startElement = document.getElementById(startID);
> +      if (!(startElement.parentNode == popup))

startelement.parentNode != popup

@@ +980,4 @@
>        var endElement = null;
>        if (endID) {
>          endElement = document.getElementById(endID);
> +        if (!(endElement.parentNode == popup))

use !=

::: browser/components/preferences/selectBookmark.js
@@ +1,1 @@
> +

Please remove this added newline

::: toolkit/components/search/nsSearchService.js
@@ +222,5 @@
>   * @throws resultCode
>   */
>  function ERROR(message, resultCode) {
> +  if (true)
> +    throw new Error(SEARCH_LOG_PREFIX + message);

just throw, without the if (true)

::: toolkit/content/globalOverlay.js
@@ +104,1 @@
>  });

These 2 parenthesis should have been removed as well... This is likely to break everything

::: toolkit/modules/moz.build
@@ +189,4 @@
>      'Color.jsm',
>      'Console.jsm',
>      'css-selector.js',
> +    'DateTimePickerHelper.jsm',    

please remove the added trailing spaces
Attachment #8946621 - Flags: review?(mak77) → review-
(Assignee)

Comment 25

a year ago
Thanks for the review I will make the changes and do follow what you said for submitting patch from next time.
(Assignee)

Comment 26

a year ago
Attachment #8948113 - Flags: review?(mak77)
(Assignee)

Updated

a year ago
Attachment #8946621 - Attachment is obsolete: true
Comment on attachment 8948113 [details] [diff] [review]
replaced NS_ASSERT and removed debug.js

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

Thanks, there are still a few things to fixup

::: browser/components/feeds/FeedConverter.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/debug.js");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");

You may have to hg rebase your patch on top of a more recent mozilla-central, indeed this code changed recently (last week) and the diff should not appear in your patch.
Provided you setup your env using ./mach bootstrap (that should take care of setting up hg properly), you can "hg pull central", use "hg wip" to check the current heads and changesets, then use "hg rebase -s your_changeset -d central". Finally update to the tip.
If instead you are using Mercurial Queues, you should be able to just qpop the change, update the tree, qpush the change and qref.

::: browser/components/places/content/controller.js
@@ +935,4 @@
>      if (!this._hasRemovableSelection())
>        return;
>  
> +    throw new Error("Must supply Transaction Name");

This is wrong now, you should remove aTxnName as argument and remove this line as well.
And also remove the txnName argument from _removeRowsFromBookmarks

::: browser/components/places/content/places.js
@@ +962,5 @@
>     */
>    _clean: function VM__clean(popup, startID, endID) {
>      if (endID)
> +      if (!startID)
> +        throw new Error("meaningless to have valid endID and null startID");

if (endID && !startID)

@@ +975,5 @@
>          endElement = document.getElementById(endID);
> +        if (endElement.parentNode != popup)
> +        throw new Error("endElement is not in popup");
> +        if (!endElement)
> +        throw new Error("endID does not correspond to an existing element");

these 2 should be properly indented

::: browser/components/places/content/treeView.js
@@ +649,4 @@
>  
>    // nsINavHistoryResultObserver
>    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> +    if (this._result, "Got a notification but have no result!");

this looks wrong
Attachment #8948113 - Flags: review?(mak77) → review-
(Assignee)

Comment 28

a year ago
(In reply to Marco Bonardo [::mak] from comment #27)
> Comment on attachment 8948113 [details] [diff] [review]
> replaced NS_ASSERT and removed debug.js
> 
> Review of attachment 8948113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, there are still a few things to fixup
> 
> ::: browser/components/feeds/FeedConverter.js
> @@ +4,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +ChromeUtils.import("resource://gre/modules/debug.js");
> > +ChromeUtils.import("resource://gre/modules/Services.jsm");
> 
> You may have to hg rebase your patch on top of a more recent
> mozilla-central, indeed this code changed recently (last week) and the diff
> should not appear in your patch.
> Provided you setup your env using ./mach bootstrap (that should take care of
> setting up hg properly), you can "hg pull central", use "hg wip" to check
> the current heads and changesets, then use "hg rebase -s your_changeset -d
> central". Finally update to the tip.
> If instead you are using Mercurial Queues, you should be able to just qpop
> the change, update the tree, qpush the change and qref.
> 
> ::: browser/components/places/content/controller.js
> @@ +935,4 @@
> >      if (!this._hasRemovableSelection())
> >        return;
> >  
> > +    throw new Error("Must supply Transaction Name");
> 
> This is wrong now, you should remove aTxnName as argument and remove this
> line as well.
> And also remove the txnName argument from _removeRowsFromBookmarks
> 
> ::: browser/components/places/content/places.js
> @@ +962,5 @@
> >     */
> >    _clean: function VM__clean(popup, startID, endID) {
> >      if (endID)
> > +      if (!startID)
> > +        throw new Error("meaningless to have valid endID and null startID");
> 
> if (endID && !startID)
> 
> @@ +975,5 @@
> >          endElement = document.getElementById(endID);
> > +        if (endElement.parentNode != popup)
> > +        throw new Error("endElement is not in popup");
> > +        if (!endElement)
> > +        throw new Error("endID does not correspond to an existing element");
> 
> these 2 should be properly indented
> 
> ::: browser/components/places/content/treeView.js
> @@ +649,4 @@
> >  
> >    // nsINavHistoryResultObserver
> >    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> > +    if (this._result, "Got a notification but have no result!");
> 
> this looks wrong

Thanks, I will soon make the changes.
(Assignee)

Comment 29

a year ago
I am getting merge conflicts when I try to rebase . Can I get any help?
you can solve merge conflicts through tools like kdiff3. If you didn't setup a merging tool, it probably used the internal merge, that adds conflict markers in the files it notified, then you have to resolve those conflicts and mark them as resolved using hg resolve.
I usually go through kdiff3, but maybe it's late for that, in your case.
In the worst case, you can always restart from the latest patch attached here.
(Assignee)

Comment 31

a year ago
> In the worst case, you can always restart from the latest patch attached
> here.
I didn't get this line Can you please explain again?
I just meant that if you should end up corrupting the tree during your experiments with merges, you could always restart with a clean tree and re-download the patch from here. But I suggest to keep learning about merges, since it can be useful for the future.
(Assignee)

Comment 33

a year ago
ok, thanks a lot.
(Assignee)

Updated

a year ago
Attachment #8948113 - Attachment is obsolete: true
(Assignee)

Comment 34

a year ago
Posted patch bug-1431050-v5.patch (obsolete) — Splinter Review
I tried to resolve the merge conflict and rebase was successful and made changes that were mentioned
Attachment #8948661 - Flags: review?(mak77)
Comment on attachment 8948661 [details] [diff] [review]
bug-1431050-v5.patch

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

We are almost there, there are a few unexpected merge artifacts though

::: browser/components/preferences/selectBookmark.js
@@ +1,1 @@
> +s//* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

the initial "s" here looks like a mistyped char

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +9,5 @@
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PromiseUtils.jsm");

These should not be changed, they should keep being ChromeUtils.import

@@ +10,5 @@
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PromiseUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "NS_ASSERT", "resource://gre/modules/debug.js");

this line should be removed.
Likely an artifact of the merge.

::: toolkit/content/globalOverlay.js
@@ +3,4 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  function closeWindow(aClose, aPromptFunction) {
> +  let { AppConstants } = Components.utils.import("resource://gre/modules/AppConstants.jsm", {});

This line should not be changed, likely another merge artifact
Attachment #8948661 - Flags: review?(mak77) → review-
(Assignee)

Comment 36

a year ago
> >  
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/PromiseUtils.jsm");
> 
> These should not be changed, they should keep being ChromeUtils.import
> 


unless and until I did it merge conflict didn't get solved.I got a diff in vim showing that Chromeutils. is changed to cu.utils
so I did it . Is there any other way to solve merge conflict then?
The merge couldn't tell whether the change was correct or not, and that's why it didn't solve it automatically and asked you to.
At that point usually the developer checks what changed in the meanwhile (using hg log or dxr.mozilla.org or searchfox.org) and applies the appropriate adaptations. But don't worry, it happens and you're not supposed to know what changed in the platform in the last few days, since it's not your primary job.
(Assignee)

Comment 38

a year ago
(In reply to Marco Bonardo [::mak] from comment #37)
> The merge couldn't tell whether the change was correct or not, and that's
> why it didn't solve it automatically and asked you to.
> At that point usually the developer checks what changed in the meanwhile
> (using hg log or dxr.mozilla.org or searchfox.org) and applies the
> appropriate adaptations. But don't worry, it happens and you're not supposed
> to know what changed in the platform in the last few days, since it's not
> your primary job.

Thank you so now should I undo my change in chromeutils? And then how can I fix merge conflict then??
I would just copy/paste the old code on top of the new one where it was wrongly modified, and amend the changes on your changeset.
You can use "hg diff -c your_changeset" to check the current diff, or just "hg diff" to check uncommitted changes before amending them.
(Assignee)

Comment 40

a year ago
Thank you
(Assignee)

Comment 41

a year ago
Posted patch bug-1431050-v6.patch (obsolete) — Splinter Review
Attachment #8948677 - Flags: review?(mak77)
(Assignee)

Updated

a year ago
Attachment #8948661 - Attachment is obsolete: true
Comment on attachment 8948677 [details] [diff] [review]
bug-1431050-v6.patch

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

Looks good to me! I'll take care of running this on the Try server.
Attachment #8948677 - Flags: review?(mak77) → review+
(Assignee)

Comment 43

a year ago
Thank you. And how can I know whether my patch is merged or not?
the bug will be marked as resolved fixed (with the commit links)
Comment on attachment 8948677 [details] [diff] [review]
bug-1431050-v6.patch

Sorry, after checking on Try, looks like you didn't convert all the entries.
In particular:
browser/base/content/tabbrowser.xml
browser/components/places/content/tree.xml
toolkit/components/search/nsSearchService.js
still contain NS_ASSERT calls
Attachment #8948677 - Flags: review+
(Assignee)

Comment 46

a year ago
Oh, I will make changes. Thank you.
Hi, are you still working on this issue?
Flags: needinfo?(manikishanghantasala)
(Assignee)

Comment 48

a year ago
Hi, yes I am working  on this and sorry that I have been inactive these days.
Flags: needinfo?(manikishanghantasala)
Posted patch bug-1431050-rebased.patch (obsolete) — Splinter Review
I'm hoping to get this over the finish line as it relates to the Console.jsm work, so I rebased manikishan's patch and made the couple of changes from Comment 45. I didn't change author info on the patch.

I just converted those couple of new cases console.assert instead of throwing because it wasn't obvious to me if they should throw and this keeps the NS_ASSERT behavior. Lmk if you'd like to change that.
Attachment #8948677 - Attachment is obsolete: true
Attachment #8955296 - Flags: review?(mak77)
Posted patch bug-1431050-rebased.patch (obsolete) — Splinter Review
There were a couple of extra imports to debug.js that were causing test failures - removed those.
Attachment #8955296 - Attachment is obsolete: true
Attachment #8955296 - Flags: review?(mak77)
Attachment #8955305 - Flags: review?(mak77)
Comment on attachment 8955354 [details] [diff] [review]
bug-1431050-rebased.patch

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

r=me with a green Try Run (at least xpcshell, mochitest-chrome, mochitest-clipboard and mochitest-browser-chrome-e10s)

::: browser/components/places/content/controller.js
@@ +939,5 @@
>     * @param   aTxnName
>     *          A name for the transaction if this is being performed
>     *          as part of another operation.
>     */
>    async remove(aTxnName) {

please remove aTxnName from here and from the javadoc

::: browser/components/places/content/treeView.js
@@ +651,5 @@
>  
>    // nsINavHistoryResultObserver
>    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> +    if (!this._result)
> +      throw new Error("Got a notification but have no result!");

all the entries in this file can be console.assert since we already bail out just below

::: toolkit/components/search/nsSearchService.js
@@ +218,1 @@
>    throw Components.Exception(message, resultCode);

does this provide a stack already? otherwise in both cases here we should probably still console.assert
Attachment #8955354 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #53)
> Comment on attachment 8955354 [details] [diff] [review]
> bug-1431050-rebased.patch
> 
> Review of attachment 8955354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with a green Try Run (at least xpcshell, mochitest-chrome,
> mochitest-clipboard and mochitest-browser-chrome-e10s)
> 
> ::: browser/components/places/content/controller.js
> @@ +939,5 @@
> >     * @param   aTxnName
> >     *          A name for the transaction if this is being performed
> >     *          as part of another operation.
> >     */
> >    async remove(aTxnName) {
> 
> please remove aTxnName from here and from the javadoc

done

> ::: browser/components/places/content/treeView.js
> @@ +651,5 @@
> >  
> >    // nsINavHistoryResultObserver
> >    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> > +    if (!this._result)
> > +      throw new Error("Got a notification but have no result!");
> 
> all the entries in this file can be console.assert since we already bail out
> just below

done

> ::: toolkit/components/search/nsSearchService.js
> @@ +218,1 @@
> >    throw Components.Exception(message, resultCode);
> 
> does this provide a stack already? otherwise in both cases here we should
> probably still console.assert

Just double checked in the Browser Console and.. it's not pretty but it does work:

function a() { throw Components.Exception("foo", 1); } a();
Shows: [Exception... "foo"  nsresult: "0x1 (<unknown>)"  location: "JS frame :: debugger eval code :: a :: line 1"  data: no]
Thinking to wait until after the merge to land this, since it's deleting the module from toolkit and throwing exceptions in places where we didn't used to.

Comment 58

a year ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd2ee90a670
Replace NS_ASSERT with conditional exceptions/console.assert and remove the debug.js module. r=mak
Keywords: checkin-needed

Comment 59

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fd2ee90a670
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1454023
You need to log in before you can comment on or make changes to this bug.