1465388 - Content CSS loaded at document_start breaks about:blank

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Giorgio Maone [:ma1]]

Attachment: Minimal test extension, loading an empty content CSS on document start

Steps to reproduce:

1. Install the attached test extension (just loading an empty CSS file as a run_at document_start css "content script".
2. Open https://bugzilla.mozilla.org (or any other non-empty web page)
3. Type about:blank in the navigation bar and hit ENTER

Expected result:
The current tab is switched to about:blank and you can see an empty page

Actual result:
the navigation bar is cleared, but the document in the current tab remains the original non-empty one (looks like the navigation has been interrupted at some point). You need to repeat step 3 for about:blank actually replacing the current document.

This is especially bad in situations where IFrames are scripted by the parent document, for instance on pages embedding WYSIWYG editors, see https://forums.informaction.com/viewtopic.php?f=7&t=24739

It's also, reportedly, a Firefox 60 regression. Unfortunately I've got no spare cycles to bisect it at this moment.

Comment 1

[Rob Wu [:robwu]]

Thanks for following up to my mail with a STR.

Bisect range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b48465720bb75ef569951454ecf3da86515544a&tochange=023a3a83d667d1c10319def65340467cb64db086

Comment 2

[Emilio Cobos Álvarez (:emilio)]

The webextensions commit in that push sounds likely to be related. Probably worth reversing it and trying?

Comment 3

[Mike Taylor [:miketaylr]]

It would be good if we could get a fix for 61, but too late for 60. bholley is gone until the 11th... so can't needinfo him just yet. Let's do that when he gets back.

Comment 4

[Bobby Holley (:bholley)]

I'll still be getting unburied for a bit. That WebExtension commit was added at Kmag's instruction, and I'm not familiar with the surrounding code. Probably best if he has a look.

Comment 5

[Rob Wu [:robwu]]

I confirmed that this failure also happens with "js" content scripts, but only on the first about:blank page load after an extension was loaded. That strongly suggests that the use of document.blockParsing is causing issues.

When I perform the test in a debug build of Firefox, the same assertion failure and log is printed as shown here: https://bugzilla.mozilla.org/show_bug.cgi?id=1407501#c19

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1465388 - Resume about:blank parser upon unblocking the document

When `document.blockParsing()` is called, the nsIParser is suspended
until the document is unblocked. For about:blank documents, this is a
nsParser.

When a document is unblocked, nsParser::ContinueInterruptedParsingAsync
is invoked, which delegates its implementation to nsIContentSink, which
is a nsHTMLContentSink for about:blank documents. Due to a missing
implementation of nsHTMLContentSink::ContinueInterruptedParsingAsync,
the parser was never resumed, causing bug 1465388 and bug 1407501.

This patch fixes the problem, by implementing the required method (and
using a load blocker to ensure that the (about:blank) document does not
finish before the parser finishes).

This patch is tested through extension tests: Currently document_start
stylesheets always activate the parser blocker, and document_start
scripts trigger the parser blocker when the script has not been
preloaded yet (e.g. at the first run).
Before this patch, the test failed due to the assertion failure as
reported in the linked bugs. After this patch, the tests pass.

Comment 7

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 9004285 [details]
Bug 1465388 - Resume about:blank parser upon unblocking the document

Henri Sivonen (:hsivonen) has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/126368a5c3ec
Resume about:blank parser upon unblocking the document r=hsivonen

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/126368a5c3ec

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Because of 1487004, let's do a backout this change and respin of nightly.

Comment 11

[Andreea Pavel [:apavel]]

This was backed out: https://hg.mozilla.org/mozilla-central/rev/a33ef251cb467a34aad71b0d94a4289addc9590f

Comment 12

[Rob Wu [:robwu]]

Need a null pointer check.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/362a6bb92de1
Resume about:blank parser upon unblocking the document r=hsivonen

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/362a6bb92de1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Too late for Fx62 at this point given that we're already building RCs, but is this something which has a severe enough user impact to keep on the radar for possible ESR60 backport down the line? It grafts cleanly.

Comment 16

[Rob Wu [:robwu]]

This bug prevents about:blank from loading correctly under certain circumstances, mainly when an extension uses a document_start stylesheet. This is a reasonable condition, and reportedly caused other issues such as WYSIWYG editors from working.

Because of this impact and the cleanly applicable patch, I recommend to backport this to ESR along with the Firefox 63 stable release. Waiting for Firefox 63 stable also allows us to detect unexpected bugs, if any.

Comment 18

[Julien Cristau [:jcristau]]

Tracking for 60.3esr per comment 16.

Comment 19

[Victor Carciu]

Verified as fixed in latest FF63(63.0b9) using Win10x64 and macOS 10.13.6. I can also confirm that I reproduced the issue in previous FF versions.

I will attach a postfix video.

Comment 20

[Victor Carciu]

Attachment: Postfix video

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR60 approval when you get a chance.

Comment 22

[Rob Wu [:robwu]]

Comment on attachment 9004285 [details]
Bug 1465388 - Resume about:blank parser upon unblocking the document

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes a bug in loading about:blank documents when an extension has declared a document_start stylesheet, such as the popular NoScript extension.

User impact if declined: When certain extensions are installed, context menus don't work in top-level about:blank documents; WYSWYG editors are broken.

Fix Landed on Version: 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a small patch that only that adds missing logic that is already present in our other document parsing implementations. This logic is triggered when extensions are used. The patch has unit tests and also fixes another known assertion failure (bug 1407501 ).

String or UUID changes made by this patch: none

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9004285 [details]
Bug 1465388 - Resume about:blank parser upon unblocking the document

Fixes broken context menus under some situations, approved for ESR 60.3.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a9577451dcc2

Comment 25

[Victor Carciu]

Verified as fixed in latest FF63(63.0b9) using Win10x64(buildid 20181006124451) and macOS 10.13.6.(buildid 20180923220427).