KhronosGroup / Vulkan-Site

Vulkan Documentation Project framework for integrated documentation site with spec, proposals, guide, and more
Other
25 stars 2 forks source link

Certain search terms will crash the site client-side #3

Closed SaschaWillems closed 1 year ago

SaschaWillems commented 1 year ago

While looking at the page during a call we noticed that certain search terms will crash the page on the client-side, with the browser showing a crash message after some time out.

Right now it looks as terms like VK_ or anything with underscores causes the crash. At least in Chrome based browsers.

oddhack commented 1 year ago

I find 'VK_' takes about 40 wall clock seconds and throws up a couple of 'unresponsive' dialogs, but does eventually return a results tab (edit: if you click through "Wait" on the dialogs). Is that consistent with what you're seeing?

Will ask on the Antora list although I expect this is a result of there being so many terms with 'VK_' in them. I also need to update the toolchain to the current Antora before asking.

oddhack commented 1 year ago

Interestingly 'Vk' stutters briefly but doesn't stall like 'VK_'.

SaschaWillems commented 1 year ago

I find 'VK_' takes about 40 wall clock seconds and throws up a couple of 'unresponsive' dialogs, but does eventually return a results tab (edit: if you click through "Wait" on the dialogs). Is that consistent with what you're seeing?

Exactly. Tested with different browsers, and after clicking through the "page is not responding" dialog I do get results.

oddhack commented 1 year ago

Asking on the Antora Zulipchat, My guess is that there are just too many matching search terms and something needs to be done in lunr to suppress searching when it's just very common prefixes, but no clue how to do that.

oddhack commented 1 year ago

It does sound like there are some logic errors in Antora's Lunr search extension which may be at least partly responsible.

oddhack commented 1 year ago

Tracking in https://gitlab.com/antora/antora-lunr-extension/-/issues/63

SaschaWillems commented 1 year ago

It looks like lunr is pretty much unmaintained. So I took a quick look at this, and changed search-ui.js to simple return from the problematic function:

  /**
   * Taken and adapted from: https://github.com/olivernn/lunr.js/blob/aa5a878f62a6bba1e8e5b95714899e17e8150b38/lib/tokenizer.js#L24-L67
   * @param lunr
   * @param text
   * @param term
   * @return {{start: number, length: number}}
   */
  function findTermPosition (lunr, term, text) {
    // Early out to avoid performance problems
    return {
      start: 0,
      length: 0,
    };

    const str = text.toLowerCase();
    const len = str.length;

    for (let sliceEnd = 0, sliceStart = 0; sliceEnd <= len; sliceEnd++) {
      const char = str.charAt(sliceEnd);
      const sliceLength = sliceEnd - sliceStart;

      if ((char.match(lunr.tokenizer.separator) || sliceEnd === len)) {
        if (sliceLength > 0) {
          const value = str.slice(sliceStart, sliceEnd);
          // QUESTION: if we get an exact match without running the pipeline should we stop?
          if (value.includes(term)) {
            // returns the first match
            return {
              start: sliceStart,
              length: value.length,
            }
          }
        }
        sliceStart = sliceEnd + 1;
      }
    }

    // not found!
    return {
      start: 0,
      length: 0,
    }
  }

That code seems to be used to highlight search terms. So disabling it (like above) the search still works as before, but the search terms in the search result window are no longer highlighted. I think that's a good compromise if the search is now responsible.

image image

Not sure how to fix this on our side, but we probably have to override that file in our UI bundle.

P.S. : You can easily test this by changing above file in your browser.

oddhack commented 1 year ago

Great! I'll update the UI bundle (hopefully that's all that will be needed; it's a bit tricky and possibly we'll need to rebuild the antora lunr package). Ideally we could get this pushed upstream, with an option antora.yml could pass in to enable the behavior.

gpx1000 commented 1 year ago

18 fixes this. Please confirm here:

https://gpx1000.github.io/Vulkan-Site/spec/latest/index.html

gpx1000 commented 1 year ago

18 fixed this.