Fix parser-time document.write() in service worker mode

[ato]

The blob URL workaround for iframe documents was also applied to document.write() calls made during parsing time causing the existing document content to be discarded.

Only use the blob workaround when document.open() or document.write() actually replace the document.

Fixes #234

---

With this patch the test case from #234, the Sydney 2000 Olympics site and https://h5p.org/presentation all work.

[ikreymer]

Nice work and great analysis!
One note, the conditions for open and write replacement are slightly different:

 !oldDocumentElement || thisObj.documentElement !== oldDocumentElement;

and

oldDocumentElement !== null &&
          $wbDocument.documentElement !== oldDocumentElement

Is this intentional? should they be consistent, is the null check needed at all?

[ato]

It was intentional and was trying to cover cases like open(); open(); close() but after thinking about it again I realised it could be simplified by making _docOpenReplacedDocument sticky and then:

After open() replaced the document, then documentElement will be null. So we don't actually need to check for change, just for null.

After write() replaced the document, then documentElement could either be null (it wrote zero chars) or non-null (it wrote something) so we need to check if it changed value.

For both open() and write() if documentElement was null both before and after the call then we don't necessarily know if document was replaced, but in that case there is no existing content we need to keep, so doing the blob workaround probably doesn't hurt. But not doing blob workaround might miss a replacement and leave the service worker broken.

[ato]

I realised this:

document.write('<script>...</script>');

will have document.close() injected into it, effectively:

document.write('<script>...; document.close();</script>')

which means the document.close() override is actually re-entrant! So we need to ensure the wombat._writeBuff global is cleared before we call orig_doc_write otherwise it can infinitely recurse.

[ikreymer]

It was intentional and was trying to cover cases like open(); open(); close() but after thinking about it again I realised it could be simplified by making _docOpenReplacedDocument sticky and then:

After open() replaced the document, then documentElement will be null. So we don't actually need to check for change, just for null.

Makes sense - I wonder if we could simplify it even further.. The document.open() call is never actually needed, and is sort of a nop, both when loading and not when loading.
Since we're checking for wasLoading, we basically know that document was cleared if and only if it was not loading...
I think might be able to get rid of _docOpenReplacedDocument altogether this way.

[ikreymer]

Makes sense - I wonder if we could simplify it even further.. The document.open() call is never actually needed, and is sort of a nop, both when loading and not when loading.

I guess an exception is calling: document.open() followed by document.close() after a document has loaded, which will clear the document and then writeBuff is empty anyway, so nothing needs to be done.

[ikreymer]

Added some suggested simplification here: https://github.com/nla/wombat/pull/1/changes

[ato]

Oh! I was surprised that worked, but of course it does because after load open() sets readyState back to loading and then write() changes documentElement. Well that's certainly much nicer!

I was trying to think of other ways to make it fail. Maybe if there's code that mixes open/write/close with DOM-construction. I tried this:

document.open();
document.appendChild(document.createElement('html'));
document.write('test');
document.close();

But fortunately the write does nothing in Chrome and in Firefox it actually throws "DOMException: An attempt was made to use an object that is not, or is no longer, usable". So at least that particular pattern we don't have to worry about.

I think I'm pretty happy with this now. Especially after your further simplifications.

[ikreymer]

Oh! I was surprised that worked, but of course it does because after load open() sets readyState back to loading and then write() changes documentElement. Well that's certainly much nicer!

Ha, I actually didn't realize this either, so it works but for slightly different reasons than I initially thought, maybe should adjust the comments!
It seems like the three states are:

1. Initial document loading: oldDocumentElement is non null and unchanged after close, no blob replacement needed
2. New document loading: document.open() reset oldDocumentElement to null, document.write() filled buffer, so close() creates new document: replacement needed.
3. New document loading: document.open() never called, document.write() reset buffer. Since readyState still at 'complete', replacement needed.

I thought it would be:
4) document.write() or document.open() called while readyState !== 'loading', replace the whole thing, but this state doesn't exist because a) write() is always buffered and b) document.open() resets state to 'loading', but this doesn't actually exist.

[ikreymer]

Based on the above, there is one additional simplification: don't actually need to check readyState at all, can just check oldDocument !== newDocument, and always first call native write(), then possibly do blob replacement.

The calling native write() is needed no matter what because subsequent code might access the results of the previous write, and that can happen whether document.open() was called or not, e.g.:

iframe.contentDocument.open();
iframe.contentDocument.write("<div></div>");
iframe.contentDocument.close();
iframe.contentDocument.querySelector("div")
...

and

iframe.contentDocument.write("<div></div>");
iframe.contentDocument.close();
iframe.contentDocument.querySelector("div");
...

should behave the same way. The new fix addresses this.

[ikreymer]

Note on browser: In my testing, Firefox has the same issue as Chrome, but only incognito mode, but not in regular mode, while Safari is actually fine. Probably not worth extra detection and should always apply the blob override

[ato]

Passes my test cases in Chrome, Firefox and Safari.

The only downside I see is this means in the blob case a written <script> will always get executed twice. But since the entire document is discarded scenarios where that actually matters seem unlikely.