1193572 - More nsXULPrototypeElement::Deserialize() cleanups

Status: RESOLVED FIXED

(Core :: XUL, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Don't use return values of failing calls in XUL deserialize methods.

These are all pretty simple.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe40a45f6e7

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - nsXULPrototypeElement::Deserialize should use fallible SetCapacity.

I think this prevents a crash if we end up with a bogus cache value.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Don't re-initialize mType for XUL proto elements.

These are already set in the constructor.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Make script element ownership more direct.

Rather than keep around a raw pointer |script| and depend on the fact
that |child| will keep alive the same object, make |script| strong,
then pass the reference off to |child| when we're finished with it.

I removed the comment about deleting |script| on failure because the
existing stack smart pointers already are doing this.

Also fix a tiny style nit.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8646654 [details] [diff] [review]
part 1 - Don't use return values of failing calls in XUL deserialize methods.

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

::: dom/xul/nsXULElement.cpp
@@ +2277,5 @@
>  
>      // Read Node Info
>      uint32_t number = 0;
>      nsresult rv = aStream->Read32(&number);
> +    if (NS_FAILED(rv)) return rv;

if (NS_WARN_IF(NS_FAILED(rv))) return rv;

@@ +2285,5 @@
>      }
>  
>      // Read Attributes
> +    rv = aStream->Read32(&number);
> +    if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +2297,5 @@
>  
>          nsAutoString attributeValue;
>          for (uint32_t i = 0; i < mNumAttributes; ++i) {
> +            rv = aStream->Read32(&number);
> +            if (NS_FAILED(rv)) return rv;

ditto

@@ +2306,5 @@
>  
>              mAttributes[i].mName.SetTo(ni);
>  
> +            rv = aStream->ReadString(attributeValue);
> +            if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +2308,5 @@
>  
> +            rv = aStream->ReadString(attributeValue);
> +            if (NS_FAILED(rv)) return rv;
> +            rv = SetAttrAt(i, attributeValue, aDocumentURI);
> +            if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +2313,5 @@
>          }
>      }
>  
> +    rv = aStream->Read32(&number);
> +    if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +2321,5 @@
>          mChildren.SetCapacity(numChildren);
>  
>          for (uint32_t i = 0; i < numChildren; i++) {
> +            rv = aStream->Read32(&number);
> +            if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +2333,5 @@
>                  child->mType = childType;
>  
> +                rv = child->Deserialize(aStream, aProtoDoc, aDocumentURI,
> +                                        aNodeInfos);
> +                if (NS_FAILED(rv)) return rv;

ditto

@@ +2341,5 @@
>                  child->mType = childType;
>  
> +                rv = child->Deserialize(aStream, aProtoDoc, aDocumentURI,
> +                                        aNodeInfos);
> +                if (NS_FAILED(rv)) return rv;

s/NS_FAILED(rv)/NS_WARN_IF(NS_FAILED(rv))/

@@ +2839,5 @@
>                                  nsXULPrototypeDocument* aProtoDoc,
>                                  nsIURI* aDocumentURI,
>                                  const nsTArray<nsRefPtr<mozilla::dom::NodeInfo>> *aNodeInfos)
>  {
> +    return aStream->ReadString(mValue);

maybe here it's better to do:

nsresult rv = aStream->ReadString(mValue);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8646656 [details] [diff] [review]
part 3 - Don't re-initialize mType for XUL proto elements.

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

what about if here http://mxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#2409
you add:

MOZ_ASSERT(child->mType == childType);

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96f6f2f2ed32
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b16a669ddc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/35bb18f83595
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd45ba4bf79

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Andrea Marchesini (:baku) from comment #6)
> MOZ_ASSERT(child->mType == childType);

Added.

I also added the WARN things all over the place.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96f6f2f2ed32
https://hg.mozilla.org/mozilla-central/rev/9b16a669ddc7
https://hg.mozilla.org/mozilla-central/rev/35bb18f83595
https://hg.mozilla.org/mozilla-central/rev/1fd45ba4bf79