Closed Bug 857102 Opened 7 years ago Closed 7 years ago

Make nsNodeInfoManager::GetNodeInfo infallible

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(5 files, 9 obsolete files)

34.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
23.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
41.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
35.16 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Currently there are two failure paths (in the one that returns a pointer): testing that the result of "new" is not null, which is unneeded; and verifying that PL_HashTableAdd succeeded.  The only failure path in PL_HashTableAdd is the call to PL_HashTableRawAdd.  The only failure paths there are if allocTable or allocEntry return null.  These are set by PL_NewHashTable().  The nsNodeInfoManager code currently uses the defaults, which may be fallible, but as far as I can see, writing trivial wrappers that translate to malloc() and free() instead of PR_MALLOC, PR_NEW, and PR_Free should make them infallible.

I've been told we really want to switch to a different hash table type entirely, but that doesn't seem like it has to be a blocker for this.  If that's preferred, I could do that instead if someone points me in the right direction.
Attached patch PatchSplinter Review
Everything but the nsNodeInfoManager.cpp change should be just removing now-redundant error paths.  Tell me if this is the wrong approach, or if the bug is a bad idea, or whatever.

Try: https://tbpl.mozilla.org/?tree=Try&rev=276ae81518ca
Attachment #732351 - Flags: review?(bzbarsky)
Comment on attachment 732351 [details] [diff] [review]
Patch

nsDOMAttributeMap::GetItemAt no longer needs an nsresult outparam.  Followup but or patch on fixing that?

nsDOMAttributeMap::GetAttrNodeInfo no longer needs the aError outparam.  Neither does nsDOMAttributeMap::GetNamedItemNS.  The null-check in nsDOMAttributeMap::RemoveNamedItemNS can also go away.  Again, followup bug or patch is fine.

NS_NewXMLCDataSection is now infallible.  It could be changed to just return an already_AddRefed<CDATASection>.  Again, followup.

I believe the same is true for NS_NewXMLProcessingInstruction, since do_GetAtom is infallible.  And NS_NewXMLStylesheetProcessingInstruction.

r=me with those followups filed or fixed.
Attachment #732351 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #2)
> nsDOMAttributeMap::GetAttrNodeInfo no longer needs the aError outparam. 
> Neither does nsDOMAttributeMap::GetNamedItemNS.  The null-check in
> nsDOMAttributeMap::RemoveNamedItemNS can also go away.  Again, followup bug
> or patch is fine.

By the null-check, you mean the NS_ERROR_DOM_NOT_FOUND_ERR path?  Why can that go away?  I see a bunch of paths in GetAttrNodeInfo that will return null.
This was actually my goal for the original patch.
Attachment #732802 - Flags: review?(bzbarsky)
Does not remove the NS_ERROR_DOM_NOT_FOUND_ERR path in RemoveNamedItemNS, per comment 3.
Attachment #732803 - Flags: review?(bzbarsky)
> By the null-check, you mean the NS_ERROR_DOM_NOT_FOUND_ERR path? 

Yes, and you're right that it needs to stay.  ;)
Comment on attachment 732802 [details] [diff] [review]
Patch part 2 -- Make NS_NewTextNode and nsIDocument::CreateTextNode infallible

Could we make it return already_AddRefed<nsTextNode> ?
Comment on attachment 732803 [details] [diff] [review]
Patch part 3 -- Make various GetNodeInfo callers infallible

Here, can we also make NS_NewXMLProcessingInstruction and NS_NewXMLStylesheetProcessingInstruction return the actual type?
Comment on attachment 732802 [details] [diff] [review]
Patch part 2 -- Make NS_NewTextNode and nsIDocument::CreateTextNode infallible

Waiting for a patch with the change to the return type.
Attachment #732802 - Flags: review?(bzbarsky) → review-
Comment on attachment 732803 [details] [diff] [review]
Patch part 3 -- Make various GetNodeInfo callers infallible

Likewise.
Attachment #732803 - Flags: review?(bzbarsky) → review-
Comment on attachment 732804 [details] [diff] [review]
Patch part 3 -- Make NS_NewCommentNode and nsIDocument::CreateCommentNode infallible

And here.
Attachment #732804 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #10)
> Waiting for a patch with the change to the return type.

So how should I deal with the problem of

error: conversion from ‘already_AddRefed<nsTextNode>’ to non-scalar type ‘nsCOMPtr<nsIContent>’ requested

?  Did you wind up fixing this on m-c yet?  (I didn't try rebasing my patches yet.)
Flags: needinfo?(bzbarsky)
Just import bug 857645 under your patches for now?  I'm going to check it in to m-i in then next several hours, but it presumably won't get to m-c until tomorrow.
Flags: needinfo?(bzbarsky)
Okay, I can compile with that -- but in fact, should I just make these into constructors?  Ms2ger reports that WebKit has new Comment(Document*, const String&), which seems a lot nicer than NS_NewCommentNode(nsNodeInfoManager*)!  I dunno if there's any point in making a version that takes Document*, since you can just use CreateComment() etc., but making it a constructor is still nicer.
Flags: needinfo?(bzbarsky)
In case you want me to not change to a constructor, here are the existing patches.
Attachment #732802 - Attachment is obsolete: true
Attachment #734598 - Flags: review?(bzbarsky)
Flags: needinfo?(bzbarsky)
Attachment #732803 - Attachment is obsolete: true
Attachment #734599 - Flags: review?(bzbarsky)
> but in fact, should I just make these into constructors? 

If they're never fallible anymore (e.g. never get passed null nodeinfo managers, which I think is the only failure mode) then yes, please!
Attachment #734598 - Attachment is obsolete: true
Attachment #734598 - Flags: review?(bzbarsky)
Attachment #734599 - Attachment is obsolete: true
Attachment #734599 - Flags: review?(bzbarsky)
New part, because why not?  Looks safe-ish to me.  The only catch is now we'll crash if passed a null node manager, and previously we'd return an error, but the callers look sane enough, I hope.  You'd know better than me if any of the callers here might pass null.

I'm not totally sure the change from do_QueryInterface() to forget() in nsHTMLDataTransfer.cpp is correct.

Is there a way to do the nsParserUtils.cpp change without NS_ADDREF, or is this the best way?

Try, linux64 unit tests: https://tbpl.mozilla.org/?tree=Try&rev=639d118cb115
Try, cross-platform build: https://tbpl.mozilla.org/?tree=Try&rev=e92163643eea
Attachment #734665 - Flags: review?(bzbarsky)
Fixes stupid mistake -- I didn't duplicate the existing assert (and more importantly, SetIsDOMBinding();) into the new constructor.
Attachment #734656 - Attachment is obsolete: true
Attachment #734656 - Flags: review?(bzbarsky)
Attachment #735135 - Flags: review?(bzbarsky)
Attachment #734657 - Attachment is obsolete: true
Attachment #734657 - Flags: review?(bzbarsky)
Attachment #735136 - Flags: review?(bzbarsky)
Attachment #734658 - Attachment is obsolete: true
Attachment #734658 - Flags: review?(bzbarsky)
Comment on attachment 735136 [details] [diff] [review]
Patch part 3, v3 -- Make various GetNodeInfo callers infallible

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

::: content/xml/document/src/nsXMLContentSink.cpp
@@ +1170,5 @@
>      rv = AddContentAsLeaf(cdata);
>      DidAddContent();
>    }
>  
>    return NS_SUCCEEDED(rv) ? DidProcessATokenImpl() : rv;

If cdata could have been null, rv would have been uninitialized here. But that can't happen, so remove the if (cdata) block and declare rv when it's first assigned to.
Fixes a further bug where I returned a raw pointer instead of already_AddRefed<>.  Why is there an implicit conversion here?

Now the patchset builds *and* starts up without crashing locally, so hopefully it works!
Attachment #735136 - Attachment is obsolete: true
Attachment #735136 - Flags: review?(bzbarsky)
Attachment #735151 - Flags: review?(bzbarsky)
The fifth part should still be good -- it didn't need to be updated further.

Linux64 unit tests: https://tbpl.mozilla.org/?tree=Try&rev=802af30703d5
All-platform build: https://tbpl.mozilla.org/?tree=Try&rev=c23c975f6ca6
Attachment #735152 - Flags: review?(bzbarsky)
> Why is there an implicit conversion here?

I think it was meant to make it easy to do:

  nsIFoo* foo = something;
  NS_ADDREF(foo);
  return foo;

but maybe we should revisit it... it's been the source of many a bug.
Comment on attachment 735135 [details] [diff] [review]
Patch part 2, v3 -- Make NS_NewTextNode and nsIDocument::CreateTextNode infallible

>+++ b/content/html/content/src/HTMLOptionElement.cpp
>+    nsRefPtr<nsTextNode> textContent =
>+      new nsTextNode(option->NodeInfo()->NodeInfoManager());
>     textContent->SetText(aText.Value(), false);

I believe this could also be:

  nsRefPtr<nsTextNode> textContent =
    option->OwnerDoc()->CreateTextNode(aText.Value());

right?  If so, might be nice to start moving in that direction....  Either way, though.  If we do want to do this, a separate bug is fine.

r=me
Attachment #735135 - Flags: review?(bzbarsky) → review+
Comment on attachment 735151 [details] [diff] [review]
Patch part 3, v4 -- Make various GetNodeInfo callers infallible

Comment 25 has a nit worth addressing.

r=me with that.
Attachment #735151 - Flags: review?(bzbarsky) → review+
Comment on attachment 735152 [details] [diff] [review]
Patch part 4, v3 -- Make NS_NewCommentNode and nsIDocument::CreateCommentNode infallible

>+  Comment(already_AddRefed<nsINodeInfo> aNodeInfo)

Does anything actually call this constructor?  If not, can we just get rid of it?

I suppose the same thing can be asked about textnode and cdata section, actually.

r=me
Attachment #735152 - Flags: review?(bzbarsky) → review+
Comment on attachment 734665 [details] [diff] [review]
Patch part 5 -- Make NS_NewDocumentFragment etc. infallible

Again, are there any callers of the nodeinfo version of the ctor?

r=me wither way
Attachment #734665 - Flags: review?(bzbarsky) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.