Closed Bug 749482 Opened 9 years ago Closed 9 years ago

Adjust assertion for RemoteImageData

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(1 file, 4 obsolete files)

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)
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()?
(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.
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.");
(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.
(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.
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)
*sigh* correct patch this time.
Attachment #619052 - Attachment is obsolete: true
Attachment #619052 - Flags: review?(roc)
Attachment #619066 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/72ff84aaf59a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.