Closed Bug 1465388 Opened Last year Closed Last year

Content CSS loaded at document_start breaks about:blank

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox-esr52 unaffected, firefox-esr6063+ verified, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 verified)

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 63+ verified
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- verified

People

(Reporter: mao, Assigned: robwu)

References

()

Details

(Keywords: regression)

Attachments

(3 files)

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.
Thanks for following up to my mail with a STR.

Bisect range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b48465720bb75ef569951454ecf3da86515544a&tochange=023a3a83d667d1c10319def65340467cb64db086
Blocks: 1438974
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Priority: -- → P1
The webextensions commit in that push sounds likely to be related. Probably worth reversing it and trying?
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.
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.
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
Blocks: 1407501
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
Assignee: kmaglione+bmo → rob
Status: NEW → ASSIGNED
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 on attachment 9004285 [details]
Bug 1465388 - Resume about:blank parser upon unblocking the document

Henri Sivonen (:hsivonen) has approved the revision.
Attachment #9004285 - Flags: review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/126368a5c3ec
Resume about:blank parser upon unblocking the document r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/126368a5c3ec
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1487004
Because of 1487004, let's do a backout this change and respin of nightly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
Flags: needinfo?(rob)
Need a null pointer check.
Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/362a6bb92de1
Resume about:blank parser upon unblocking the document r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/362a6bb92de1
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Flags: needinfo?(rob)
Flags: in-testsuite+
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.
Flags: needinfo?(rob)
Duplicate of this bug: 1485282
Tracking for 60.3esr per comment 16.
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.
Status: RESOLVED → VERIFIED
Attached image Postfix video
Please nominate this for ESR60 approval when you get a chance.
Flags: needinfo?(rob)
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
Flags: needinfo?(rob)
Attachment #9004285 - Flags: approval-mozilla-esr60?
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.
Attachment #9004285 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Verified as fixed in latest FF63(63.0b9) using Win10x64(buildid 20181006124451) and macOS 10.13.6.(buildid 20180923220427).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.