Crash: Simultaneous accesses to 0x… but modification requires exclusive access.

regexident commented 4 years ago

I'm getting a crash of …

Simultaneous accesses to 0x… but modification requires exclusive access.

… at this line …

let mutableAttributedString = try NSMutableAttributedString(
    data: data,
    options: options,
    documentAttributes: &documentAttributes
)

… in a SwiftUI app when trying to convert the following markdown string …

"E = mc<sup>2</sup>"

… to an NSAttributedString (while doing the same with "E = mc^2" works just fine).

Userland stack-trace:

What's apparently not causing the issue

I tried isolating/reproducing the issue in a unit test in a fork of CommonMarkAttributedString:

func testCrash() throws {
    let commonmark = #"""
    E = mc<sup>2</sup>
    """#

    #if canImport(UIKit)
    let _ = try NSAttributedString(commonmark: commonmark)
    #elseif canImport(AppKit)
    let _ = try NSAttributedString(commonmark: commonmark)
    #endif
}

func testCrash2() throws {
    let html = #"""
    <p>E = mc<!-- raw HTML omitted -->2<!-- raw HTML omitted --></p>\n
    """#

    #if canImport(UIKit)
    let _ = try NSAttributedString(html: html, attributes: [:])
    #elseif canImport(AppKit)
    let _ = try NSAttributedString(html: html, attributes: [:])
    #endif
}

But there things worked just fine. No crash. 😕

So next I tried replacing documentAttributes: &documentAttributes with documentAttributes: nil, since it is actually not used afterwards, and changed Nio's dependency to that fork, hoping it might solve the crasher. It didn't.

How to reproduce

Create new single-view SwiftUI iOS app.
Add https://github.com/mattt/CommonMarkAttributedString.git SPM package.

Change the contents of ContentView.body to …

import SwiftUI

import CommonMarkAttributedString

struct ContentView: View {
    var body: some View {
        let string = try! NSAttributedString(
            commonmark: "E = mc<sup>2</sup>"
        )

        return Text("Hello, World!")
    }
}

Start the app in iOS simulator and watch it crash.

mattt commented 4 years ago

Great writeup as always, @regexident! I have half a mind to start an Etsy shop that sells framed prints of your bug reports 😍

The good news is that I'm pretty sure what the underlying cause is. This old-school documentation from Apple has a good writeup:

Multicore considerations: Since OS X v10.4, NSAttributedString has used WebKit for all import (but not for export) of HTML documents. Because WebKit document loading is not thread safe, this has not been safe to use on background threads. For applications linked on OS X v10.5 and later, if NSAttributedString imports HTML documents on any but the main thread, the use of WebKit is transferred to the main thread via performSelectorOnMainThread:withObject:waitUntilDone:. This makes the operation thread safe, but it requires that the main thread be executing the run loop in one of the common modes. This behavior can be overridden by setting the value of the standard user default NSRunWebKitOnAppKitThread to either YES (to obtain the new behavior regardless of linkage) or NO (to obtain the old behavior regardless of linkage).

Although this was last updated in 2014, I believe the diagnosis is still correct. As for the prescribed treatment... it may work, but it's far from optimal.

I must admit that when I was first writing this library, I had a vague memory of HTML → NSAttributedString having thread safety issues. But in an effort to get something out, I punted on that. That's my bad for not taking the time to look into this more — at least enough to document this limitation in the README or open an issue. So I apologize for that.

Where do we go from here?

Like I said, I'm not a fan of the workaround prescribed in Apple's documentation. I think the best long-term solution will involve finding or creating a new library (preferably, using Markup) for doing the HTML conversion.

regexident commented 4 years ago

Ugh, I didn't mean to close this. 🙄

Great writeup as always, @regexident! I have half a mind to start an Etsy shop that sells framed prints of your bug reports 😍

😆 Having been on the receiving end of quite a few horribly impudent and lazy issues myself I just try my best to be a good open source citizen. 😅

Where do we go from here?

Like I said, I'm not a fan of the workaround prescribed in Apple's documentation. I think the best long-term solution will involve finding or creating a new library (preferably, using Markup) for doing the HTML conversion.

Probably the best solution, yeah. Or maybe just skip HTML entirely and go straight from Markdown to NSAttributedString?

Either way, lemme know if I can help you here in any way.

Doing things ourselves would also give us more control over how the output is formatted. The current formatting doesn't quite match the output produced by most common Markdown editors/viewers and the paragraph margins are a bit off, I think.

(also cough https://github.com/mattt/CommonMarkAttributedString/issues/6 cough 😏)

mattt commented 4 years ago

Probably the best solution, yeah. Or maybe just skip HTML entirely and go straight from Markdown to NSAttributedString?

To clarify, I'm proposing that, for each HTMLBlock and InlineHTML CommonMark node, to use this proposed library to parse and convert the raw HTML to an NSAttributedString. We probably don't want to convert CommonMark to HTML and then to NSAttributedString if we can avoid it.

Either way, lemme know if I can help you here in any way.

Will do! This seems "fun" enough that I may well have a sketch of this within the next couple days.

Mas0nSun commented 3 years ago

I have the same problem. It is possible that the main thread is causing this problem This is my code. It will cause a crash. ❌ Thread 1: Simultaneous accesses to 0x7f882bc1ec40, but modification requires exclusive access

    func updateUIView(_ label: UILabel, context: Context) {
        let parser = MarkdownParser()
        let html = parser.html(from: string)
        if let attributedString = try? NSAttributedString(
            data: Data(html.utf8),
            options: [.documentType: NSAttributedString.DocumentType.html],
            documentAttributes: nil
        ) {
            label.attributedText = attributedString
        }
    }

It works if I put them into the global queue. ✅

    func updateUIView(_ label: UILabel, context: Context) {
        let parser = MarkdownParser()
        let html = parser.html(from: string)
        DispatchQueue.global().async {
            if let attributedString = try? NSAttributedString(
                data: Data(html.utf8),
                options: [.documentType: NSAttributedString.DocumentType.html],
                documentAttributes: nil
            ) {
                DispatchQueue.main.async {
                    label.attributedText = attributedString
                }
            }
        }
    }

helje5 commented 3 years ago

A nice, I also debugged this as part of Nio. It doesn't seem to be a threading issue (both accesses are in one stack), but really a concurrent Swift access thing due to some weird WebKit/SwiftUI nesting. (I think it may happen because the thing is nested within a Swift constructor, but not quite sure)

I think NSXMLParser is based on libxml2 and supports HTML (or is that just NSXMLDocument?). If so, one could probably use that to parse some basic HTML instead.

Oh, and I forgot: This isn't actually a bug in CommonMarkAttributedString, but in NSAttributedString(data:options:documentAttributes:), just in case someone bothers and wants to file a Radar.

mattt / CommonMarkAttributedString

Crash: Simultaneous accesses to 0x… but modification requires exclusive access. #7

What's apparently not causing the issue

How to reproduce