Last Comment Bug 749482 - Adjust assertion for RemoteImageData
: Adjust assertion for RemoteImageData
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-26 19:12 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-05-07 16:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Alter assertion inside SetRemoteImageData (899 bytes, text/plain)
2012-04-26 19:12 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details
Really fix assertion in SetRemoteImageData (988 bytes, patch)
2012-04-26 20:42 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Really fix assertion in SetRemoteImageData v2 (903 bytes, patch)
2012-04-26 21:16 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Really fix assertion in SetRemoteImageData v3 (903 bytes, patch)
2012-04-27 08:36 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Really fix assertion in SetRemoteImageData v4 (1.00 KB, patch)
2012-04-27 09:08 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-04-26 19:12:22 PDT
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.
Comment 1 Bas Schouten (:bas.schouten) 2012-04-26 20:42:13 PDT
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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-26 21:08:30 PDT
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) 2012-04-26 21:14:26 PDT
(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) 2012-04-26 21:16:57 PDT
Created attachment 618924 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v2

This patch does what you suggested.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-26 21:19:59 PDT
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) 2012-04-27 07:07:27 PDT
(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) 2012-04-27 08:35:33 PDT
(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) 2012-04-27 08:36:12 PDT
Created attachment 619052 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v3
Comment 9 Bas Schouten (:bas.schouten) 2012-04-27 09:08:13 PDT
Created attachment 619066 [details] [diff] [review]
Really fix assertion in SetRemoteImageData v4

*sigh* correct patch this time.

Note You need to log in before you can comment on or make changes to this bug.