592 bytes, text/html
12.72 KB, text/plain
7.54 KB, text/plain
3.66 KB, patch
|Details | Diff | Splinter Review|
Created attachment 619793 [details] testcase (requires pref) (requires focus) (crashes Firefox) 1. Set user_pref("full-screen-api.allow-trusted-requests-only", false); 2. Load the testcase. Result: GetPresContext is called on a deleted pres shell, usually causing a crash (which by coincidence looks like a null pres context deref).
In nsCanvasRenderingContext2DAzure::DrawOrMeasureText 3188 nsIPresShell* presShell = GetPresShell(); ... 3204 nsComputedDOMStyle::GetStyleContextForElement(..., presShell) ... 3248 presShell->GetPresContext(), GetStyleContextForElement causes a flush that releases the last reference to the pres shell. Maybe the local |presShell| should be an nsCOMPtr. (If you make a change in nsCanvasRenderingContext2DAzure::DrawOrMeasureText, be sure to make a corresponding change in the non-azure version, since it also crashes.) Or maybe this is a bug in how full-screen works. The Mac OS X Lion transition was added recently in bug 639705. But I first saw this crash on Linux, which doesn't use that transition.
Created attachment 619796 [details] gdb output (misleading) Opt: bp-f2bb9670-e86d-4b31-98c6-c2c672120501
> GetStyleContextForElement causes a flush Yes. presShell should be a COMPtr here. And the GetPresContext() call needs to be null-checked.
Created attachment 620092 [details] [diff] [review] attempt to fix I couldn't reproduce the crash on Windows, I tried both a trunk build and current beta, debug and optimised. Could you let me know if this patch fixes the problem for you? Thanks!
Comment on attachment 620092 [details] [diff] [review] attempt to fix Review of attachment 620092 [details] [diff] [review]: ----------------------------------------------------------------- This should work... ::: content/canvas/src/nsCanvasRenderingContext2D.cpp @@ +2929,5 @@ > nscoord totalWidthCoord; > > // calls bidi algo twice since it needs the full text width and the > // bounding boxes before rendering anything > + NS_ASSERTION(presShell, "presShell is Null"); This assertion is really pointless since we already null-checked it in this method and nothing modifies this local variable. ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +3240,5 @@ > nscoord totalWidthCoord; > > // calls bidi algo twice since it needs the full text width and the > // bounding boxes before rendering anything > + NS_ASSERTION(presShell, "presShell is Null"); Ditto
Created attachment 620122 [details] [diff] [review] attempt to fix Changed the assertions to check the PresContext, I assume they could be Null some how, but this is not checked anywhere. Shouldn't have anything to do with this crash though.
The prescontext can't be null either :-)
Created attachment 620131 [details] [diff] [review] attempt to fix No assertions at all this time.
(In reply to Nick Cameron [:nrc] from comment #5) > Created attachment 620092 [details] [diff] [review] > attempt to fix > > I couldn't reproduce the crash on Windows, I tried both a trunk build and > current beta, debug and optimised. > > Could you let me know if this patch fixes the problem for you? Thanks! Jesse: could you test the patch on your setup please? Thanks!
Nick: if this is the right fix do we need it on the ESR-10 branch and/or aurora/beta? Or is this a recent regression?
I'm not sure if it is a recent regression in terms of causing a crash, but I checked the code I am modifying, and it has been around for a long time. The full screen api is changing quite a bit recently, so that could cause this, and it therefore could be recent. Perhaps Jesse could let us know if it occurs on v10 or beta?
Any news on this bug? Should we land the patch? I have no idea if the patch fixes the problem, but it does correct a bug, and I'm pretty sure it doesn't make anything worse. It would be great if someone who can reproduce the bug can test the patch, or could help me to reproduce the bug.
Comment on attachment 620131 [details] [diff] [review] attempt to fix This patch fixes the bug for me :)
The testcase doesn't crash ESR for me, but that doesn't necessarily mean ESR is unaffected.
Created attachment 625521 [details] [diff] [review] patch no changes (other than setting r=roc), carrying r=roc
nrc / Jesse: does this crash affect branches? If so, we probably want to be ready for branch landings ASAP after landing on trunk, and we'd want to schedule those landings carefully, so that we don't end up landing on the branch right before a relase and suddenly breaking stuff, or right after and zero-daying ourselves. So -- before this lands on trunk, we probably want to know its applicability for our branches and have patches ready to land there within a few days of the trunk-landing. (if separate patches are needed) (also, deleting '[needs feedback from jruderman]' since it looks like that was addressed in comment 14 - comment 15)
What is the risk here?
Absolutely minimal. We're just turning a regular pointer into an nsCOMPtr which adds and releases a reference.
This patch looks like it would apply to the ESR-10 branch as well as aurora/beta. Even if we're not reproducing the crash with this specific testcase on those branches it would be safest to take this patch anyway -- there may be other ways to encourage a PresShell to die prematurely.
Too late to get this in to beta (code freeze is tomorrow) so tracking for aurora & esr 14
Fixed bitrot (by applying patch w/ fuzz) & softened the commit message to be more coy about the exploitability, and pushed: https://hg.mozilla.org/mozilla-central/rev/5581c3b2a95c (Less concerned about Comment 22 now that we're in a better spot in the release cycle.)
Comment on attachment 625521 [details] [diff] [review] patch I'm just going ahead & requesting approval for aurora/beta/esr per Comment 21. [Approval Request Comment] Fix Landed on Version: 16 Risk to taking this patch (and alternatives if risky): Minimal (see comment 20) User impact if declined: Possible crashiness / exploitability - see comment 21. String or UUID changes made by this patch: none
Verified the crash pre-fix and the lack of crash in the 6/14 nightly trunk build.
Comment on attachment 625521 [details] [diff] [review] patch Low risk, sg:crit fix. Approved for all branches.
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c15c260e7b9 https://hg.mozilla.org/releases/mozilla-beta/rev/dc903fbd5f39 https://hg.mozilla.org/releases/mozilla-esr10/rev/65ffec7cdb1d
(In reply to Daniel Holbert [:dholbert] from comment #27) > https://hg.mozilla.org/releases/mozilla-esr10/rev/65ffec7cdb1d Sorry, that ESR link was the wrong cset -- here's the correct one: https://hg.mozilla.org/releases/mozilla-esr10/rev/c0a3383e56d7
The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should this be reopened?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #29) > The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should > this be reopened? Sorry, latest is actually: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.5esrpre) Gecko/20120531 Firefox/10.0.5esrpre I take it we need a new ESR Nightly to be built for me to verify this.
Sorry for the bugspam -- I just realized there is a 10.0.6pre from today in the same folder -- testing it now. Sorry for the confusion.
Retested Firefox 10.0.6esrpre 2012-06-19 and the testcase does not crash. Apologies again for the churn. Is regressionwindow-wanted still necessary for this bug?
Taking the lack of follow-up to comment 12 as a "no" to resolving qawanted/regressionwindow-wanted. Please re-add if there is something more QA can do.
Out of curiosity, why not change the other PresShell references in the same files?
(In reply to Mike Hommey [:glandium] from comment #34) > Out of curiosity, why not change the other PresShell references in the same > files? I would guess they should be, but perhaps if they don't need to be, then it is better to avoid the addref/release. I don't really know. roc - do you have an opinion on this?