Last Comment Bug 750575 - Invalid read (from deleted presShell) calling canvas strokeText during full-screen transition
: Invalid read (from deleted presShell) calling canvas strokeText during full-s...
Status: VERIFIED FIXED
[sg:critical][qa+][advisory-tracking+]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks: randomstyles 379903
  Show dependency treegraph
 
Reported: 2012-04-30 18:03 PDT by Jesse Ruderman
Modified: 2012-07-20 18:24 PDT (History)
13 users (show)
dholbert: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
fixed
14+
verified


Attachments
testcase (requires pref) (requires focus) (crashes Firefox) (592 bytes, text/html)
2012-04-30 18:03 PDT, Jesse Ruderman
no flags Details
valgrind output (12.72 KB, text/plain)
2012-04-30 18:04 PDT, Jesse Ruderman
no flags Details
gdb output (misleading) (7.54 KB, text/plain)
2012-04-30 18:06 PDT, Jesse Ruderman
no flags Details
attempt to fix (4.49 KB, patch)
2012-05-01 15:18 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
attempt to fix (4.53 KB, patch)
2012-05-01 16:16 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
attempt to fix (3.58 KB, patch)
2012-05-01 16:34 PDT, Nick Cameron [:nrc]
roc: review+
jruderman: feedback+
Details | Diff | Review
patch (3.66 KB, patch)
2012-05-20 15:06 PDT, Nick Cameron [:nrc]
ncameron: review+
ncameron: feedback+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Jesse Ruderman 2012-04-30 18:03:57 PDT
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).
Comment 1 Jesse Ruderman 2012-04-30 18:04:24 PDT
Created attachment 619794 [details]
valgrind output
Comment 2 Jesse Ruderman 2012-04-30 18:04:35 PDT
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.
Comment 3 Jesse Ruderman 2012-04-30 18:06:12 PDT
Created attachment 619796 [details]
gdb output (misleading)

Opt: bp-f2bb9670-e86d-4b31-98c6-c2c672120501
Comment 4 Boris Zbarsky [:bz] 2012-04-30 18:10:47 PDT
> GetStyleContextForElement causes a flush

Yes.  presShell should be a COMPtr here.  And the GetPresContext() call needs to be null-checked.
Comment 5 Nick Cameron [:nrc] 2012-05-01 15:18:06 PDT
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 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 15:50:47 PDT
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
Comment 7 Nick Cameron [:nrc] 2012-05-01 16:16:21 PDT
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.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 16:21:08 PDT
The prescontext can't be null either :-)
Comment 9 Nick Cameron [:nrc] 2012-05-01 16:34:14 PDT
Created attachment 620131 [details] [diff] [review]
attempt to fix

No assertions at all this time.
Comment 10 Nick Cameron [:nrc] 2012-05-08 18:12:15 PDT
(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!
Comment 11 Daniel Veditz [:dveditz] 2012-05-10 13:17:10 PDT
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?
Comment 12 Nick Cameron [:nrc] 2012-05-10 20:23:23 PDT
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?
Comment 13 Nick Cameron [:nrc] 2012-05-16 15:59:30 PDT
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 14 Jesse Ruderman 2012-05-20 13:53:06 PDT
Comment on attachment 620131 [details] [diff] [review]
attempt to fix

This patch fixes the bug for me :)
Comment 15 Jesse Ruderman 2012-05-20 15:01:41 PDT
The testcase doesn't crash ESR for me, but that doesn't necessarily mean ESR is unaffected.
Comment 16 Nick Cameron [:nrc] 2012-05-20 15:06:08 PDT
Created attachment 625521 [details] [diff] [review]
patch

no changes (other than setting r=roc), carrying r=roc
Comment 17 Nick Cameron [:nrc] 2012-05-21 20:24:53 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=be00392bbc1f
Comment 18 Daniel Holbert [:dholbert] 2012-05-22 16:22:55 PDT
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)
Comment 19 Al Billings [:abillings] 2012-05-22 16:27:39 PDT
What is the risk here?
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:24:35 PDT
Absolutely minimal. We're just turning a regular pointer into an nsCOMPtr which adds and releases a reference.
Comment 21 Daniel Veditz [:dveditz] 2012-05-24 13:43:40 PDT
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.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-24 16:19:47 PDT
Too late to get this in to beta (code freeze is tomorrow) so tracking for aurora & esr 14
Comment 23 Daniel Holbert [:dholbert] 2012-06-12 10:56:58 PDT
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 24 Daniel Holbert [:dholbert] 2012-06-12 11:02:00 PDT
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
Comment 25 Al Billings [:abillings] 2012-06-14 06:23:44 PDT
Verified the crash pre-fix and the lack of crash in the 6/14 nightly trunk build.
Comment 26 Alex Keybl [:akeybl] 2012-06-14 09:48:39 PDT
Comment on attachment 625521 [details] [diff] [review]
patch

Low risk, sg:crit fix. Approved for all branches.
Comment 28 Daniel Holbert [:dholbert] 2012-06-14 10:29:28 PDT
(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
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-19 13:28:34 PDT
The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should this be reopened?
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-19 13:32:47 PDT
(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.
Comment 31 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-19 13:34:30 PDT
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.
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-19 13:39:47 PDT
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?
Comment 33 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-10 13:58:49 PDT
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.
Comment 34 Mike Hommey [:glandium] 2012-07-13 01:14:50 PDT
Out of curiosity, why not change the other PresShell references in the same files?
Comment 35 Nick Cameron [:nrc] 2012-07-14 07:47:40 PDT
(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?

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