793866 - Spell checker crash after doing mean things with contenteditable

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox) (requires focus)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

bp-aa561b6f-702b-4705-8f9b-98ca22120924

Comment 3

[Scoobidiver (away)]

On Windows: bp-38d24faf-e967-409d-b4a7-3edce2120924

Comment 4

[(no longer active)]

Aryeh can you take a look at this, please?  Thanks!

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

The patch fixes the problem locally, but the crashtest doesn't seem to work -- it passes even without the code changes.  So this really isn't properly tested.  Other than that, seems to work.  Any idea what might be wrong with the test?

No, I have no idea how UpdateCurrentDictionary() sets mSpellCheck to null.  I didn't try stepping through the whole thing.  Probably I could figure out how to get gdb to break on a particular variable's contents being changed, but it didn't seem worth the effort.

I don't feel this needs a try run, given issues with tryserver load lately.  The only possible problem is if the crashtest somehow fails on some weird platform, but it seems unlikely.

Comment 6

[(no longer active)]

Comment on attachment 669460 [details] [diff] [review]
Patch

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

This looks sane... I can't repro the crash with the test case locally either which is quite weird.  I'm not sure if I can explain this... But let's take the test anyways.

(Also agreed on no need for try runs)

Comment 7

[Jesse Ruderman]

> The patch fixes the problem locally, but the crashtest doesn't seem to work
> -- it passes even without the code changes.  So this really isn't properly
> tested.  Other than that, seems to work.  Any idea what might be wrong with
> the test?

* Make sure the test has focus when you run it.

* The crashtest harness might advance to the next page before spell check has a chance to do its thing.  (Spell check is async, right?)  See if adding class="reftest-wait" and a timeout fixes it.  If so, try to find something better than a timeout to wait for ;)

Comment 8

[(no longer active)]

(In reply to comment #7)
> > The patch fixes the problem locally, but the crashtest doesn't seem to work
> > -- it passes even without the code changes.  So this really isn't properly
> > tested.  Other than that, seems to work.  Any idea what might be wrong with
> > the test?
> 
> * Make sure the test has focus when you run it.

It does.

> * The crashtest harness might advance to the next page before spell check has a
> chance to do its thing.  (Spell check is async, right?)  See if adding
> class="reftest-wait" and a timeout fixes it.  If so, try to find something
> better than a timeout to wait for ;)

I tried adding a reftest-wait class to it and not removing it so that the test suite stops... Still no crash!

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3139381f3c4

This doesn't really seem to be tested, as noted, so setting in-testsuite? instead of + even though a test is included in the changeset.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3139381f3c4