Closed Bug 793866 Opened 9 years ago Closed 9 years ago

Spell checker crash after doing mean things with contenteditable

Categories

(Core :: Spelling checker, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jruderman, Assigned: ayg)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files)

No description provided.
Attached file stack
bp-aa561b6f-702b-4705-8f9b-98ca22120924
Crash Signature: [@ mozInlineSpellChecker::UpdateCurrentDictionary]
On Windows: bp-38d24faf-e967-409d-b4a7-3edce2120924
Crash Signature: [@ mozInlineSpellChecker::UpdateCurrentDictionary] → [@ mozInlineSpellChecker::UpdateCurrentDictionary()] [@ mozInlineSpellChecker::UpdateCurrentDictionary]
OS: Mac OS X → All
Hardware: x86_64 → All
Aryeh can you take a look at this, please?  Thanks!
Assignee: nobody → ayg
Attached patch PatchSplinter Review
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.
Attachment #669460 - Flags: review?(ehsan)
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)
Attachment #669460 - Flags: review?(ehsan) → review+
> 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 ;)
(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!
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.
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/e3139381f3c4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.