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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Gary [:streetwolf52]]

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

Comment 1

[Gary [:streetwolf52]]

UA: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 2

[Gary [:streetwolf52]]

Site works fine in IE11 and MS Edge.

Comment 3

[:Gijs (he/him)]

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).

Comment 4

[Gary [:streetwolf52]]

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.

Comment 5

[Gary [:streetwolf52]]

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.

Comment 6

[Gary [:streetwolf52]]

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

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

Wfm on Firefox 54.01 (Mac) with Zoom Page WE 5.1 installed. I didn't try any other add-ons.

Comment 8

[Gary [:streetwolf52]]

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.

Comment 9

[u287251]

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

Comment 10

[Alice0775 White]

[Tracking Requested - why for this release]:

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

Regressed by:Bug 1333990

Comment 11

[Kris Maglione [:kmag]]

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.

Comment 12

[:Gijs (he/him)]

(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).

Comment 13

[Kris Maglione [:kmag]]

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 15

[Mike Taylor [:miketaylr]]

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.

Comment 16

[Gary [:streetwolf52]]

(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.

Comment 17

[Mike Taylor [:miketaylr]]

(thanks for checking!)

Comment 18

[u287251]

My understanding is that this is a content script injection issue in webext framework ("document_start"?), not a uBO-specific issue.

Comment 19

[Mike Taylor [:miketaylr]]

Shell, what should we do with this bug? THanks.

Comment 20

[Mike Taylor [:miketaylr]]

(Redirecting to Andy, I think shell is PTO. Is comment #18 correct?)

Comment 21

[Andy McKay]

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.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Not tracking as we just released 55 and it wasn't blocker anyway.

Comment 23

[Peja Stija]

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.

Comment 24

[mapx]

https://bugzilla.mozilla.org/show_bug.cgi?id=1396226

Comment 25

[Rob Wu [:robwu]]

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?

Comment 26

[Mark]

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.

Comment 27

[Mark]

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.

Comment 28

[Rob Wu [:robwu]]

Attachment: document.write-content-script-1379148.zip

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.

Comment 30

[mapx]

Probably the same issue here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1397399

Comment 32

[Kris Maglione [:kmag]]

Attachment: Bug 1379148: Part 1 - Allow blocking only non-script-created parsers.

Review commit: https://reviewboard.mozilla.org/r/181982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181982/

Comment 33

[Kris Maglione [:kmag]]

Attachment: Bug 1379148: Part 2 - Don't block script-created parsers when executing content scripts.

Review commit: https://reviewboard.mozilla.org/r/181984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181984/

Comment 34

[Kris Maglione [:kmag]]

Attachment: Bug 1379148: Part 3 - Inject scripts synchronously if they're already available.

Review commit: https://reviewboard.mozilla.org/r/181986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181986/

Comment 35

[Kris Maglione [:kmag]]

Attachment: Bug 1379148: Part 4 - Add tests for document.write() with document_start content script present.

Review commit: https://reviewboard.mozilla.org/r/181988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181988/

Comment 36

[Shane Caraveo (:mixedpuppy)]

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

Comment 37

[Shane Caraveo (:mixedpuppy)]

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.

Comment 38

[Shane Caraveo (:mixedpuppy)]

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

Comment 39

[Henri Sivonen (:hsivonen)]

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?

Comment 40

[Kris Maglione [:kmag]]

(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.

Comment 41

[Kris Maglione [:kmag]]

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 42

[Kris Maglione [:kmag]]

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.

Comment 43

[Kris Maglione [:kmag]]

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

Comment 44

[Kris Maglione [:kmag]]

Comment on attachment 8910542 [details]
Bug 1379148: Part 1 - Allow blocking only non-script-created parsers.

Approval Request Comment: See comment 42

Comment 45

[Rob Wu [:robwu]]

(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.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

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

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bee577676298
https://hg.mozilla.org/mozilla-central/rev/ba22a83959a0
https://hg.mozilla.org/mozilla-central/rev/14ab0648ea91
https://hg.mozilla.org/mozilla-central/rev/14298b92236c

Comment 48

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Since we're forcing everyone to webextensions for 57, this could cause more problems...

Comment 49

[Andy McKay]

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.

Comment 50

[Kris Maglione [:kmag]]

What they said.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

OK, make senses :)
Thanks!
Should be in 57b3

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df77a4c1b591
https://hg.mozilla.org/releases/mozilla-beta/rev/b06cc382f74d
https://hg.mozilla.org/releases/mozilla-beta/rev/e9235f4e44ee
https://hg.mozilla.org/releases/mozilla-beta/rev/de68fff98d0c

Comment 53

[marius.santa]

Attachment: pin pop-up not working.gif

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.