Closed Bug 1209446 Opened 9 years ago Closed 9 years ago

CompositorOGL::mFrameInProgress not reset properly when early leaving ::BeginFrame

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: tworaz666, Assigned: tworaz666)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150925195032

Steps to reproduce:

In case the rect object in CompositorOGL::BeginFrame is empty the function lives almost immediately. Unfortunately it does not reset the mFrameInProgress flag to false. As a result when such situation happens in debug builds the code will terminate due to an assertion failure in the next ::BeginFrame call.


Actual results:

Gecko terminated due to a failing assertion MOZ_ASSERT(!mFrameInProgress, "frame still in progress (should have called EndFrame");.


Expected results:

The assert should not fail.
Attachment #8667187 - Flags: review?(vladimir)
Keywords: checkin-needed
Comment on attachment 8667187 [details] [diff] [review]
0001-Bug-1209446-Reset-mFrameInProgress-flag-to-false-whe.patch

Better yet -- move the mFrameInProgress = true setting to below this block, with a comment saying something like "We're about to actually draw a frame".

This mFrameInProgress stuff seems odd -- shouldn't EndFrame also check that there's a frame actually in progress, and if not, early return?  Poking :nical (he knows the OGL compositor best these days)...
Attachment #8667187 - Flags: feedback?(nical.bugzilla)
Attachment #8667187 - Attachment is obsolete: true
Attachment #8667187 - Flags: review?(vladimir)
Attachment #8667187 - Flags: feedback?(nical.bugzilla)
Attachment #8667328 - Flags: review?(nical.bugzilla)
Comment on attachment 8667328 [details] [diff] [review]
0001-Bug-1209446-Make-sure-mFrameInProgress-flag-is-set-t.patch

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

This is a slight improvement and a very simple patch, so r+ but I'd like to go further (as a followup if you want) because I feel uneasy about having the the requirement that EndFrame is called if and only if BeginFrame succeeded with no way for the caller to know if BeginFrame succeeded.
We should either enforce that if one calls BeginFrame then it has to call EndFrame, or have BeginFrame return a boolean and enforce that EndFrame is called if and only if BeginFrame returned true.

Piotr, do you want to make a patch for this? Also, please update your mercurial config following the instructions here https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F so that your patches have the right formatting.
Attachment #8667328 - Flags: review?(nical.bugzilla) → review+
Assignee: nobody → tworaz666
Thanks for the review Nicolas. I'm afraid I won't have the time in the next couple of days at least, to make sure EndFrame is always called.

Unfortunately I'm using git, not mercurial. The company I currently work for uses EmbedLite gecko port for their browser. Since EmbedLite is using git it's easier to take git patches and convert them to hg. Such workflow seems to be at least partially supported by mozilla, see: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#I%27m_all_used_to_Git_but_how_can_I_provide_Mercurial-ready_patches. The problem with the commit message is in the fact that it uses dash instead double colon to separate bug number from the commit description, right?
https://hg.mozilla.org/mozilla-central/rev/4f2620f7822d
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.