Closed Bug 615149 Opened 9 years ago Closed 9 years ago

Crash under nsAccDocManager::ClearDocCache if I quit while printing

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: jruderman, Assigned: ginnchen+exoracle)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

Attached file testcase
1. Load the testcase.

2. Enable accessibility, e.g. by pasting the following into the js console:

Components
  .classes["@mozilla.org/accessibilityService;1"]
  .getService(Components.interfaces.nsIAccessibleRetrieval);

3. Initiate a print-to-file. (A small Firefox window should appear with a progress bar.)

4. Quit Firefox before the printing progress bar reaches 100%.

Result:

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 612
###!!! ASSERTION: PL_DHASH_ENTRY_IS_LIVE(entry): 'PL_DHASH_ENTRY_IS_LIVE(entry)', file pldhash.c, line 721

Crash [@ nsRefPtr<nsDocAccessible>::~nsRefPtr]
Whiteboard: [sg:critical?]
Attached file stack traces
I can't reproduce the crash in opt.
David, can you investigate this?
Assignee: nobody → bolterbugz
I didn't recreate this on Solaris with GNOME a11y on.
Instead, I got an assertion at cairo-hash.c:195 hash_table->live_entries == 0.

For step 2, I pasted the line to web console, and I got permission denied.
I guess I don't need this step since I've GNOME a11y on.
Checked log, I saw

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 611
###!!! ASSERTION: PL_DHASH_ENTRY_IS_LIVE(entry): 'PL_DHASH_ENTRY_IS_LIVE(entry)', file pldhash.c, line 720

So, I think I did reproduce the bug.
At
http://hg.mozilla.org/mozilla-central/file/824f8a023254/accessible/src/base/nsAccDocManager.cpp#l502

We called aDocAccessible->Shutdown();
It will do NotifyOfDocumentShutdown(), and remove the doc accessible from cache.

So when we return PL_DHASH_REMOVE for ClearDocCacheEntry(), the entry was already removed, crash.
I think return PL_DHASH_NEXT will solve the problem.

Normally we shutdown all the documents in other places and ClearDocCacheEntry() is never called.
Attached patch patch (obsolete) — Splinter Review
Assignee: bolterbugz → ginn.chen
Status: NEW → ASSIGNED
Attachment #494318 - Flags: review?
Attachment #494318 - Flags: review? → review?(surkov.alexander)
Er, is it safe to do remove while enumerator is running?
Attachment #494318 - Attachment is obsolete: true
Attachment #494318 - Flags: review?(surkov.alexander)
(In reply to comment #8)
> Er, is it safe to do remove while enumerator is running?

It should be, we may run out of hash but it looks like this should happen in the case of PL_DHASH_REMOVE (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.c#735).

We need somebody who knows xpcom hash code to check.
(In reply to comment #9)
> (In reply to comment #8)
> > Er, is it safe to do remove while enumerator is running?
> 
> It should be, we may run out of hash but it looks like this should happen in
> the case of PL_DHASH_REMOVE
> (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.c#735).
> 
> We need somebody who knows xpcom hash code to check.

According to the comment in nsTHashtable.h,
RawRemoveEntry() is safe to use during enumeration, but RemoveEntry() is not.
NotifyOfDocumentShutdown() uses RemoveEntry().
Attached patch patchSplinter Review
Other ideas?
Attachment #494328 - Flags: review?(surkov.alexander)
other idea would be ignore NotifyOfDocumentShutdown() when we are shut down the accessibility service. For that we should move gIsShutdown = PR_TRUE before nsAccDocManager::Shutdown(), it should be safe but I didn't check.

Your approach is fine I think, though that while makes me a little bit worry because what if document didn't removed itself from document cache, then we get infinite loop. That shouldn't ever happen though.

Please let me know what you prefer.
I'd prefer my approach, I think it is easier to maintain.
Comment on attachment 494328 [details] [diff] [review]
patch


>+  *(nsDocAccessible**)aUserArg = aDocAccessible;

use reinterpret_cast

>+void nsAccDocManager::ClearDocCache()

void on own line
Attachment #494328 - Flags: review?(surkov.alexander) → review+
(In reply to comment #3)
> David, can you investigate this?

Apparently no need when I'm surrounded by awesome :)

(Thanks Ginn!)
Comment on attachment 494328 [details] [diff] [review]
patch

Approving patch to land on trunk (please fix Alexander's nits).
Attachment #494328 - Flags: approval2.0+
Landed on Ginn's behalf: http://hg.mozilla.org/mozilla-central/rev/4661521b737c
(Try server ran green)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.