750575 - Invalid read (from deleted presShell) calling canvas strokeText during full-screen transition

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: valgrind output

Comment 2

[Jesse Ruderman]

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]

Attachment: gdb output (misleading)

Opt: bp-f2bb9670-e86d-4b31-98c6-c2c672120501

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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]]

Attachment: 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) (email my personal email if necessary)]

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]]

Attachment: 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) (email my personal email if necessary)]

The prescontext can't be null either :-)

Comment 9

[Nick Cameron [:nrc]]

Attachment: attempt to fix

No assertions at all this time.

Comment 10

[Nick Cameron [:nrc]]

(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]]

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]]

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]]

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]

Comment on attachment 620131 [details] [diff] [review]
attempt to fix

This patch fixes the bug for me :)

Comment 15

[Jesse Ruderman]

The testcase doesn't crash ESR for me, but that doesn't necessarily mean ESR is unaffected.

Comment 16

[Nick Cameron [:nrc]]

Attachment: patch

no changes (other than setting r=roc), carrying r=roc

Comment 17

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=be00392bbc1f

Comment 18

[Daniel Holbert [:dholbert]]

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 - ex-MoCo]]

What is the risk here?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Absolutely minimal. We're just turning a regular pointer into an nsCOMPtr which adds and releases a reference.

Comment 21

[Daniel Veditz [:dveditz]]

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]

Too late to get this in to beta (code freeze is tomorrow) so tracking for aurora & esr 14

Comment 23

[Daniel Holbert [:dholbert]]

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]]

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 - ex-MoCo]]

Verified the crash pre-fix and the lack of crash in the 6/14 nightly trunk build.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 625521 [details] [diff] [review]
patch

Low risk, sg:crit fix. Approved for all branches.

Comment 27

[Daniel Holbert [:dholbert]]

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

Comment 28

[Daniel Holbert [:dholbert]]

(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

[u279076]

The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should this be reopened?

Comment 30

[u279076]

(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

[u279076]

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

[u279076]

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

[u279076]

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]]

Out of curiosity, why not change the other PresShell references in the same files?

Comment 35

[Nick Cameron [:nrc]]

(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?