Closed Bug 1379148 Opened 6 years ago Closed 5 years ago

document.write does not synchronously modify a document if an extension has content scripts at document_start

Categories

(WebExtensions :: Untriaged, defect, P1)

56 Branch
x86_64
Windows 10
defect

Tracking

(firefox-esr52 unaffected)

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected

People

(Reporter: streetwolf52, Assigned: kmag)

References

()

Details

(Keywords: regression)

Attachments

(6 files)

The problem site is https://secure.seeyourchart.com/INDEX.aspx

I have all Webextension add-ons. I can only login to the site above if I disable all but three of my add-ons. To test just click on Login. If things are working you should get a small window about entering a PIN. If you don't get this window then you have the problem I have.

Browser console shows this when I click on Login and things aren't working:

TypeError: doc.forms.rsForm is undefined RemoteScripting.js:556:2

Here are my add-ons. The first three don't cause my problem. Enabling any of the others do.  

Enhancer for YouTube™ 2.0.21
History Cleaner 1.2.2
Textarea Cache Lite 2.2.8
AutoplayStopper 0.9.8
bitwarden - Free Password Manager 1.13.5
uBlock Origin/webext 1.13.7rc0  <----- Actually this is a hybrid WE.
Zoom Page WE 5.1
UA: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Component: General → Untriaged
Site works fine in IE11 and MS Edge.
Andy, who can take a look at this type of issue? Given that three different add-ons individually manage to cause this issue, I'm tempted to think there might be an actual webextensions issue here, but I don't really know how to find out (and don't have the time to just bumble my way through myself).
Component: Untriaged → Extension Compatibility
Flags: needinfo?(amckay)
Unfortunately I only started using the site yesterday so I don't know if it worked correctly at some point in the past.  Since three of my WE add-ons don't affect the site there must be something different about the rest of my add-ons that do.
Actually it's 4 of my add-ons that cause the problem.  The first 3 on my add-on list don't affect the site.
I have a thread going at Mozillzine if you are interested in other folks experience with the site in question.

http://forums.mozillazine.org/viewtopic.php?f=23&t=3031584
Wfm on Firefox 54.01 (Mac) with Zoom Page WE 5.1 installed. I didn't try any other add-ons.
All the folks over at Mozillazine that tested it out for me are on Fx56.  Seems only Fx56 is affected, maybe fx55.  Haven't tried Google Chrome yet.  That might be interesting since Chrome basically uses Webextensions too.
I can reproduce with only uBO-webext-hybrid.[1]

This is what I observed:

- Open dev tools, select debugger, enable "Pause on uncaught exceptions".

- Click the "Login" button.

- As a result, the debugger should stop on the faulty source code line, RemoteScripting.js:556:2
    - I observe that doc.forms['rsForm'] is actually defined, even though the exception was thrown as a result of it NOT being defined.

- Set a breakpoint on that line.

- Set the exception setting to "Ignore exceptions".

- Click "play" button resume execution.

- Click the "Login" button again, the breakpoint should be hit, i.e. javascript execution will stop before the line is executed.

- Click "play" button to resume execution. Result: worked fine.

I don't know what to make of this, seems like there is a timing issue.


[1] https://github.com/gorhill/uBlock/releases/download/1.13.7rc1/uBlock0.webext.xpi
[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bb325d01c4219c4ecf49c2e8830eb762320358ec&tochange=291b34abf2f47f7a168652aec4c6c54dce7492db

Regressed by:Bug 1333990
Blocks: 1333990
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kmaglione+bmo)
Keywords: regression
We block parsing when the document element is inserted into a document that has content scripts until those content scripts are ready to load. In this case, the page is creating a document with document.open() and expecting the HTML it appends via document.write() to be available immediately after document.close(), but that's not a valid assumption when the parser is blocked.

I suppose we could ignore the request to block the parser for script-created parsers, but I suspect we'll get complaints from add-on authors if we do.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #11)
> I suppose we could ignore the request to block the parser for script-created
> parsers, but I suspect we'll get complaints from add-on authors if we do.

What do the other browsers do here? Seems people can't reproduce in Chrome with similar webextensions (based on the mozillazine thread).
Flags: needinfo?(amckay) → needinfo?(kmaglione+bmo)
I don't know. They probably just synchronously load and parse the scripts, like we used to do, but we're not going back down that path.

I suppose we could also just load the scripts without returning to the event loop when they're already cached. Then, depending on whether or not we skip blocking the parser, we'll get inconsistent behavior either on the extension or the web content side.
Flags: needinfo?(kmaglione+bmo)
As I understand it, this issue may break some pages for users who have uBlock installed. And maybe other addons. 
Agreed with Kris that we are unlikely to back out the work in bug 1333990.  In that bug we have a tag asking for developer documentation. Maybe it would help to define the changes in documentation for add-on developers. 

Kris, are you wontfixing this bug? Do we have a way to tell how widespread it might be?
Flags: needinfo?(kmaglione+bmo)
Hi Gary, uBlock origin just released a new (beta) version: https://addons.mozilla.org/en-US/firefox/addon/ublock-origin/versions/beta?page=1#version-1.13.9b0

Does this issue still reproduce with that installed? Thanks.
Flags: needinfo?(garyshap)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #15)
> Hi Gary, uBlock origin just released a new (beta) version:
> https://addons.mozilla.org/en-US/firefox/addon/ublock-origin/versions/
> beta?page=1#version-1.13.9b0
> 
> Does this issue still reproduce with that installed? Thanks.

Unfortunately the new uBO beta doesn't fix the problem.
Flags: needinfo?(garyshap)
(thanks for checking!)
My understanding is that this is a content script injection issue in webext framework ("document_start"?), not a uBO-specific issue.
Shell, what should we do with this bug? THanks.
Flags: needinfo?(sescalante)
(Redirecting to Andy, I think shell is PTO. Is comment #18 correct?)
Flags: needinfo?(sescalante) → needinfo?(amckay)
It could be. We'd have to try and figure out how big a problem this is to prioritize it. It seems like its pretty rare, but I'm make a guess on that. Moving over to the WebExtensions triage for triage and a better title.
Component: Extension Compatibility → WebExtensions: Untriaged
Flags: needinfo?(amckay)
Product: Firefox → Toolkit
Not tracking as we just released 55 and it wasn't blocker anyway.
Other site breaking since the switch from legacy to web-extensions.
As discussed here : https://github.com/gorhill/uBlock/issues/2949

STR:
- Start with a blank profile.
- Navigate to https://www.bpostbanque.be/portal/start.asp (site loads fine)
- Install uBlock Origin web-extension.
- Site mentioned is blank.
- Disable uBlock Origin for the site mentioned (whitelist)
- Site mentioned remains loading blank.
See Also: → 1396226
I've rephrasing the title to be a bit more descriptive.

I think that this should have a higher priority. It breaks websites in the wild whenever there is a content_script that matches an about:blank frame. That is basically any Firefox browser instance with adblock-type extensions.

Such scripts are already likely in the ScriptCache, so can we consider having a synchronous script injection branch for document_start scripts in script-created parsers?
Blocks: 1309926
Summary: TypeError: doc.forms.rsForm is undefined RemoteScripting.js:556:2 → document.write does not synchronously modify a document if an extension has content scripts at document_start
Nightly 57 is also affected!

Confirmed problem still exists in Firefox Nightly 57.0a1 (2017-09-06) (64-bit) Build ID 20170906100107 under Windows 7 (64-bit).

I had a new profile, navigated to https://www.bpostbanque.be/portal/Start.asp and saw logon page and cookie notice. Restarted Firefox Nightly, added uBlock Origin, tyen went to the same website, but this time only the cookie notice shows.

Removed uBlock Origin. Web site comes up. Restarted Nightly on the new profile. Installed AdBlock Plus beta (WebExtensions). Went to website but this time the tab is blank.

Neither AdBlock Plus nor uBlock Origin indicated that they had blocked anything.
OOps, ended comment too soon. Repeated by whitelisting site (so AdBlock Plus or uBlock Origin will be inactive), and still the site shows just a blank tab. Only disabling (in about:addins) or removing the blocker would allow site to come up.
Here is a self-contained test case.
The zip file contains an extension with a very simple content script at document_start,
and a HTML file that has a checkbox to toggle whether to open a frame and reset the HTML parser, or to just open the frame.

When the parser is reset, the bug as reported here occurs.

And I also notice that with document.write it is possible to run scripts before the content script has had a chance to run... That is bad for extensions who want to sanitize the global window object before the page has had a chance to tamper with it.
Priority: -- → P1
Assignee: nobody → kmaglione+bmo
Probably the same issue here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1397399
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8910543 [details]
Bug 1379148: Part 2 - Don't block script-created parsers when executing content scripts.

https://reviewboard.mozilla.org/r/181984/#review187330
Attachment #8910543 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8910544 [details]
Bug 1379148: Part 3 - Inject scripts synchronously if they're already available.

https://reviewboard.mozilla.org/r/181986/#review187332

::: toolkit/components/extensions/ExtensionContent.jsm:334
(Diff revision 1)
> +
> +    let scripts = scriptPromises.map(promise => promise.script);
> +    // If not all scripts are already available in the cache, block
> +    // parsing and wait all promises to resolve.
> +    if (!scripts.every(script => script)) {
> +      let scriptsPromise = Promise.all(scriptPromises);

nit: scriptsPromise and scriptPromises are a touch close, changing scriptPromises to compilePromises may aid readability.
Attachment #8910544 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8910545 [details]
Bug 1379148: Part 4 - Add tests for document.write() with document_start content script present.

https://reviewboard.mozilla.org/r/181988/#review187334
Attachment #8910545 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8910542 [details]
Bug 1379148: Part 1 - Allow blocking only non-script-created parsers.

https://reviewboard.mozilla.org/r/181982/#review187454

I guess this is the closest we can do within this architecture while keeping the default non-dangerous. How do other browsers deal with this case?
Attachment #8910542 - Flags: review?(hsivonen) → review+
(In reply to Henri Sivonen (:hsivonen) from comment #39)
> I guess this is the closest we can do within this architecture while keeping
> the default non-dangerous. How do other browsers deal with this case?

I'm not sure, but I think they probably synchronously load the scripts, like we used to do. I don't want to go back down that path, though.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee57767629862c4b4bfa6258918551ed7d82998
Bug 1379148: Part 1 - Allow blocking only non-script-created parsers. r=hsivonen

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba22a83959a06a457c8a22d0cd9521eaed220bac
Bug 1379148: Part 2 - Don't block script-created parsers when executing content scripts. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/14ab0648ea9194b7ad84ecf7a330c9bbdadfb303
Bug 1379148: Part 3 - Inject scripts synchronously if they're already available. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/14298b92236cfcfdd0b9dc43d8077e576f775b4d
Bug 1379148: Part 4 - Add tests for document.write() with document_start content script present. r=mixedpuppy
Comment on attachment 8910544 [details]
Bug 1379148: Part 3 - Inject scripts synchronously if they're already available.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1333990
[User impact if declined]: This causes some websites to break in the presence of extensions with content scripts that run at document_start in about:blank sub-frames. uBlock Origin is one such extension.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Described in comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: These changes simply prevent us from blocking the HTML parser when loading scripts for documents created via document.open(). This corner case is already problematic, and these changes can only make it less so.
[String changes made/needed]: None.
Attachment #8910544 - Flags: approval-mozilla-beta?
Comment on attachment 8910543 [details]
Bug 1379148: Part 2 - Don't block script-created parsers when executing content scripts.

Approval Request Comment: See comment 42
Attachment #8910543 - Flags: approval-mozilla-beta?
Comment on attachment 8910542 [details]
Bug 1379148: Part 1 - Allow blocking only non-script-created parsers.

Approval Request Comment: See comment 42
Attachment #8910542 - Flags: approval-mozilla-beta?
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #40)
> (In reply to Henri Sivonen (:hsivonen) from comment #39)
> > I guess this is the closest we can do within this architecture while keeping
> > the default non-dangerous. How do other browsers deal with this case?
> 
> I'm not sure, but I think they probably synchronously load the scripts, like
> we used to do. I don't want to go back down that path, though.

In Chromium, document_start scripts are run synchronously.
document_end and document_idle scripts used to run synchronously too, but Chrome is going in the direction of running them asynchronously (scripts from different extensions run in different tasks).
For more details, see https://crbug.com/636655#c47 and below.
We already shipped 55 with this issue and we will ship 56 with it too.
Can we let it ride the train with 58?
Thanks
Flags: needinfo?(kmaglione+bmo)
[Tracking Requested - why for this release]: Since we're forcing everyone to webextensions for 57, this could cause more problems...
57 will have something like 15 million more users than 56 as add-ons like AdBlock Plus move over to WebExtensions. This improvement to content scripts will be relevant to those users.
What they said.
Flags: needinfo?(kmaglione+bmo)
OK, make senses :)
Thanks!
Should be in 57b3
Attachment #8910542 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8910543 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8910544 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This feature is verified on Firefox 58.0a1 (20170926100259) and Firefox 57.0b3 (20170925150345) under Windows 10 64-bit and Mac OS X 10.12.3.

The popup functionality works as expected.

Please see the attached video.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.