529442 - crash [@ nsWeakFrame::InitInternal(nsIFrame*) ]

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Feels similar to bug 529397 and bug 529371.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

In nsWeakFrame::InitInternal:
nsIPresShell* shell = mFrame->PresContext()->GetPresShell();

mFrame appears poisoned?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

e.g. crash stack:
http://crash-stats.mozilla.com/report/index/8565d147-5957-4eb1-9f19-1b9782091117

Comment 3

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

Yes it does. That's strange.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Hmmm. Maybe this is badness caused by nsWeakFrame(nsnull)?

In our AccessibleTreeWalker we can have this I think...
(via mState.frame = nsnull and the operator= of nsWeakFrame)

Comment 5

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

  nsIFrame *frame = mState.frame.GetFrame();
  mAccService->GetAccessible(mState.domNode, presShell, mWeakShell,
                             &frame, &mState.isHidden,
                             getter_AddRefs(mState.accessible));
  mState.frame = frame;

This looks bogus. I should have caught it before. We grab the frame from the nsWeakFrame, then we call GetAccessible (which we know might destroy the frame), then we store the (possibly now dead) frame pointer back into mState.frame.

What was that code supposed to be doing? Shouldn't we just remove those two lines before and after GetAccessible?

Comment 6

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

Oh, I see, it's because the aFrameHint parameter in GetAccessible is an in/out parameter. And sometimes GetAccessible doesn't change that parameter.

Probably the best thing to do here at the moment is change aFrameHint to be an nsWeakFrame*.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Right.

But you are also right about that last assignment, it creeped in when dbaron and I
passed the patch back and forth.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Forget comment 7 (I'm tired).

You are right, it is an in/out param.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> Probably the best thing to do here at the moment is change aFrameHint to be an
> nsWeakFrame*.

It will be nicer but it will require a uuid change I think, so I'd like to avoid it for now.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Thinking aloud. Would it make sense to always flush layout just before using our nsAccessibleTreeWalker?

Comment 11

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

That might help, but it's hard to be sure.

I think you have to change the GetAccessible interface somehow. As it is, the caller can't tell the difference between
1) The frame pointed to by aFrameHint was destroyed and the pointer is now to (poisoned) garbage
2) The frame pointed to by aFrameHint was destroyed, and a new frame was allocated at the same address, and returned in aFrameHint

i.e. the caller cannot know whether the result of aFrameHint is valid or not.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch

we could do something like this

Comment 13

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

That changes GetAccessible to return null in *aFrameHint on some paths where it left *aFrameHint unchanged before. Are you sure that's OK? If so, that's a good solution.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> That changes GetAccessible to return null in *aFrameHint on some paths where it
> left *aFrameHint unchanged before. Are you sure that's OK? If so, that's a good
> solution.

I didn't want to change the logic. You're right. Either I need to 
if (weakFrameHint.GetFrame())
  *aFrameHint = weakFrameHint.GetFrame()

or to initialize weakFrameHint by *aFrameHint.

Do I understand correct?

Comment 15

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

> if (weakFrameHint.GetFrame())
>   *aFrameHint = weakFrameHint.GetFrame()

That doesn't fix the bug. In the buggy case, weakFrameHint.GetFrame() will return null because we destroyed the frame, so you won't update *aFrameHint, so it will continue to point to a dead frame, and the caller will crash.

> or to initialize weakFrameHint by *aFrameHint.

That should work. That means if GetAccessible destroys the frame, and doesn't put a new one in weakFrameHint, *aFrameHint will return null. I hope that's OK.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 17

[Marco Zehe (:MarcoZ)]

Note that I can reproduce this crash at will by installing an add-on or by opening Tools/Add-Ons. See these reports by me:
bp-c50f7400-b893-49ce-a8e5-d44fa2091118
and
bp-b8602c11-5b93-40e9-ab54-4378c2091118

Comment 18

[Marco Zehe (:MarcoZ)]

Note that I cannot reproduce this problem with a local build. The one thing I do notice is that it takes ages (up to 5 seconds) for the add-on window to come up, but it doesn't crash. Whereas with the regular nightly build it immediately crashes when I open Tools/Add-Ons.

So to verify that the above patch fixes the bug, ideally I'd need a try-server build based off of the 1.9.2 tree.

Comment 19

[alexander :surkov (:asurkov)]

1.9.2 try-server build - https://build.mozilla.org/tryserver-builds/[e-mail]-1258538470/

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch192

Comment 21

[Marco Zehe (:MarcoZ)]

This try-server build doesn't crash for me. I also see a delay now when opening Tools/Add-Ons, but I could install Firebug 1.5xb2 (which caused one of my above quoted crashes to happen with the previous nightly), and then go back in after the restart to check for updates (which caused the other above crash with the regular nightly).

From my end this looks good.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #15)
> > if (weakFrameHint.GetFrame())
> >   *aFrameHint = weakFrameHint.GetFrame()
> 
> That doesn't fix the bug. In the buggy case, weakFrameHint.GetFrame() will
> return null because we destroyed the frame, so you won't update *aFrameHint, so
> it will continue to point to a dead frame, and the caller will crash.
> 

Right.

> > or to initialize weakFrameHint by *aFrameHint.
> 
> That should work. That means if GetAccessible destroys the frame, and doesn't
> put a new one in weakFrameHint, *aFrameHint will return null. I hope that's OK.

I think we can live with that for now.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

We can still see the file ctrl with this patch so that's a good sign.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 413035 [details] [diff] [review]
patch2

Some nice catches here thanks! I'm not sure we need two weakFrame objects in GetAccessible... I've pushed a tweaked patch to see if Marco can confirm it is still ok and I will attach if it is.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2.5 (trunk)

This keeps Alexander's catches but removes the extra weakframe object introduced in that patch.
Try server builds: https://build.mozilla.org/tryserver-builds/[e-mail]-try-22b7683077df/

Comment 26

[Marco Zehe (:MarcoZ)]

The m-c try server build looks good. Neither installing an extension nor the update UI crash. So far, so good!

I'm waiting for the 1.9.2 try-server build since that more closely matches the previous conditions.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2.5 (1.9.2 version)

Waiting for try build...

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #18)
> Note that I cannot reproduce this problem with a local build.

This is a mystery. Anyone got a good hunch? Do local builds do something different with add-on updates?

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

I suppose a better question is, do beta builds have any special config for addon updates?

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Patch 2.5 1.9.2 tryserver build:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-1a3bc8ef2e18/try-1a3bc8ef2e18-win32.zip

Comment 31

[Marco Zehe (:MarcoZ)]

Comment on attachment 413084 [details] [diff] [review]
patch 2.5 (trunk)

Also works on the 1.9.2 try-server build. r=me for the functionality. Now you guys have to figure out the code :)

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 413086 [details] [diff] [review]
patch 2.5 (1.9.2 version)

Note to 192 drivers, this will make FF 3.6 crash a lot less. (The patch simply improves the sync between our raw Frame and weak Frame state). Fix confirmed by Marco Zehe.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 413084 [details] [diff] [review]
patch 2.5 (trunk)

r=me for these v2.5 patches.

Let's land on Alexander's behalf.

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

I mean land the trunk patch, and the 192 if we get a+ ;)

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 413086 [details] [diff] [review]
patch 2.5 (1.9.2 version)

a1.9.2=dbaron

Comment 36

[aja+bugzilla]

FYI, with the tryserver linux build of patch 2.5, the crashes in bugs 529371 and 529397 still occur (though no crash reporter gets launched, so I can't confirm the stacks are still the same).

Comment 37

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

Can you attach gdb to get some kind of stack?

Hmm, I don't know if there's a way to get Linux symbols for tryserver builds...

Comment 38

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

http://hg.mozilla.org/mozilla-central/rev/26a2aacf9e3a

Closing since we fixed a crash that Marco could reproduce. Aja, if you can still crash with the trunk build, please file a new bug and request blocking1.9.2!

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Aja already filed bug 529371 and bug 529397.

Comment 40

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84464f19fac7

Comment 41

[Marco Zehe (:MarcoZ)]

Now this is getting bizarre: With nightly builds done today, which include the fixes from this bug (they landed 10 hours before the nightly builds started), I get these crashes:
mozilla-central: bp-6e723c79-9a9d-4b3d-93e5-728442091119
mozilla-1.9.2: bp-b095688d-e759-4d65-ae70-181dd2091119

All I need to do is open Tools/Add-Ons.

This did not happen while I was testing the try-server builds linked to in comment#25 and comment#30 respectively. In fact I could open the add-ons manager multiple times without problems.

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Ben, any hunches on comment #41? Build diffs?

Comment 43

[[e-mail] (:bhearsum)]

(In reply to comment #42)
> Ben, any hunches on comment #41? Build diffs?

I have no idea; the try server opt builds are basically the same configuration as the nightly builds. Perhaps it's a different patch that's caused the crash? Not sure.

Comment 44

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091121 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Comment 45

[Marco Zehe (:MarcoZ)]

Verified fixed also in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b4pre) Gecko/20091121 Namoroka/3.6b4pre (.NET CLR 3.5.30729)

Comment 46

[Daniel Veditz [:dveditz]]

Do older branches need this fix?

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #46)
> Do older branches need this fix?

No, I don't think we need to back port this use of nsWeakFrame. If we see an increase in treewalker or getaccessible crashes pre 192 we should consider it then. Thanks.

Comment 48

[David Bolter [:davidb] (NeedInfo me for attention)]

On second thought, Roc's reply to bug 504144 comment 5 is probably correct and we could backport this one too; note the nsPopupSetFrame fix is higher priority I think.

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

I just took another look at our 1.9.1 code and realize Roc's concern doesn't apply. I retract the 1.9.1 blocking request with apologies.

Comment 50

[David Bolter [:davidb] (NeedInfo me for attention)]

Clearing pre 192 requests based on last comment.

Comment 51

[David Bolter [:davidb] (NeedInfo me for attention)]

I probably should not have cleared status 1.9.1 wanted sorry (I can't set it back).