Adjust assertion for RemoteImageData

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
mozilla15
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 618895 [details]
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.
Attachment #618895 - Flags: review?(roc)
Attachment #618895 - Flags: review?(roc) → review+
(Assignee)

Comment 1

5 years ago
Created attachment 618911 [details] [diff] [review]
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.
Attachment #618895 - Attachment is obsolete: true
Attachment #618911 - Flags: review?(roc)
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()?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 618924 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v2

This patch does what you suggested.
Attachment #618924 - Flags: review?(roc)
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.");
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 619052 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v3
Attachment #618911 - Attachment is obsolete: true
Attachment #618924 - Attachment is obsolete: true
Attachment #618911 - Flags: review?(roc)
Attachment #618924 - Flags: review?(roc)
Attachment #619052 - Flags: review?(roc)
(Assignee)

Comment 9

5 years ago
Created attachment 619066 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v4

*sigh* correct patch this time.
Attachment #619052 - Attachment is obsolete: true
Attachment #619052 - Flags: review?(roc)
Attachment #619066 - Flags: review?(roc)
Attachment #619066 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/72ff84aaf59a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.