749482 - Adjust assertion for RemoteImageData

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Alter assertion inside SetRemoteImageData

RemoteImageData is allocated in the IPC code. Since this code often gets shutdown earlier than the ImageContainer, we need to unset the image data from the container. We already do this however it does trigger an assertion. This will alter the assertion, although it will become less useful since it won't guard against someone setting RemoteImageData, unsetting it, and setting different RemoteImageData. Even though this will work at the moment it was something we wanted to prevent.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Really fix assertion in SetRemoteImageData

In my hurry I tested with XPCOM_DEBUG_BREAK=warn. This should actually fix the assertions, right now there could still be an active image because it wasn't updating the active image.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand. Are we relying on mRemoteData->mWasUpdated always being true when calling SetRemoteData(nsnull)? Would it make sense to just clear mActiveImage instead of calling EnsureActiveImage()?

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> I don't understand. Are we relying on mRemoteData->mWasUpdated always being
> true when calling SetRemoteData(nsnull)? Would it make sense to just clear
> mActiveImage instead of calling EnsureActiveImage()?

I'm fine with doing that and removing the assertion. The idea behind the assertion I guess was that people don't call this when there's still an active image in the mRemoteData, I guess there's no real need for that now that we accept people calling SetRemoteData with nsnull though.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Really fix assertion in SetRemoteImageData v2

This patch does what you suggested.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 618924 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v2

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

I think this should be:

NS_ASSERTION(!mActiveImage || !aData, "No active image expected when SetRemoteImageData is called with non-null data.");
NS_ASSERTION(!mRemoteData || !aData, "No remote data expected when SetRemoteImageData is called with non-null data.");

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 618924 [details] [diff] [review]
> Really fix assertion in SetRemoteImageData v2
> 
> Review of attachment 618924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this should be:
> 
> NS_ASSERTION(!mActiveImage || !aData, "No active image expected when
> SetRemoteImageData is called with non-null data.");
> NS_ASSERTION(!mRemoteData || !aData, "No remote data expected when
> SetRemoteImageData is called with non-null data.");

I'm fine with this, but technically just looking at the current state of this function it would be fundamentally impossible to trigger the first assertion without triggering the second.

1. When mRemoteData is NULL an active image cannot be created
2. When mRemoteData is set to NULL the current active image is cleared

Hence mActiveImage cannot be non-NULL when mRemoteData is non-NULL.

I'm still alright with this if it feels more future proof for you though.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas) from comment #6)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> > Comment on attachment 618924 [details] [diff] [review]
> > Really fix assertion in SetRemoteImageData v2
> > 
> > Review of attachment 618924 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think this should be:
> > 
> > NS_ASSERTION(!mActiveImage || !aData, "No active image expected when
> > SetRemoteImageData is called with non-null data.");
> > NS_ASSERTION(!mRemoteData || !aData, "No remote data expected when
> > SetRemoteImageData is called with non-null data.");
> 
> I'm fine with this, but technically just looking at the current state of
> this function it would be fundamentally impossible to trigger the first
> assertion without triggering the second.
> 
> 1. When mRemoteData is NULL an active image cannot be created
> 2. When mRemoteData is set to NULL the current active image is cleared
> 
> Hence mActiveImage cannot be non-NULL when mRemoteData is non-NULL.
> 
> I'm still alright with this if it feels more future proof for you though.

Ignore me, my comment was silly, an active image -can- be set through non-remote-data means. Updating patch.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Really fix assertion in SetRemoteImageData v3

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Really fix assertion in SetRemoteImageData v4

*sigh* correct patch this time.

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/72ff84aaf59a