Closed Bug 1193572 Opened 6 years ago Closed 6 years ago

More nsXULPrototypeElement::Deserialize() cleanups

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

No description provided.
I think this prevents a crash if we end up with a bogus cache value.
Attachment #8646655 - Flags: review?(amarchesini)
These are already set in the constructor.
Attachment #8646656 - Flags: review?(amarchesini)
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.
Attachment #8646657 - Flags: review?(amarchesini)
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;
Attachment #8646654 - Flags: review?(amarchesini) → review+
Attachment #8646655 - Flags: review?(amarchesini) → review+
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);
Attachment #8646656 - Flags: review?(amarchesini) → review+
Attachment #8646657 - Flags: review?(amarchesini) → review+
(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.
You need to log in before you can comment on or make changes to this bug.