1605657 - Since Firefox 72, XML files don't pretty print with the React Developer Tools add-on installed

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2cad80ddf5c89ff844752e499313cdcea5b0f554&tochange=2012ad5d5af45bc7e7fec2a24b1e2472c7536d8d

This is very strange, as the only code modified is picture-in-picture code, which shouldn't be running for an XML file anyway. Are you very, very sure about this regression window?

FWIW, this looks to me like a compat issue between the react devtools and our XML pretty printer. The pretty printer turns itself off if anything modifies the DOM ( https://searchfox.org/mozilla-central/rev/8f7b017a31326515cb467e69eef1f6c965b4f00e/dom/xml/nsXMLPrettyPrinter.cpp#147 ), which the react devtools do because they insert a <script> tag from https://github.com/facebook/react/blob/b438699d3620bff236282b049204e1221b3689e9/packages/react-devtools-extensions/src/injectGlobalHook.js#L85 .

The simplest fix here would be the react/redux devtools not doing that (ie don't inject <script> tags into random XML documents...).

I don't know why it wasn't broken before... Perhaps in the "good" case there was some kind of race which caused the extension content script to run before the XML pretty printer ran, and now that's no longer the case? Also pinging :mixedpuppy in case this is something of interest to the add-ons folks.

Flags: needinfo?(mixedpuppy)

Flags: needinfo?(gingerbread_man)

Flags: needinfo?(bayyatej.dev)

Gingerbread Man

Comment 5

•

5 years ago

(In reply to :Gijs (he/him) from comment #4)

Are you very, very sure about this regression window?

To double-check, I manually downloaded and verified that the issue is reproducible with the first release with and not reproducible with the last release without.

To triple-check, I then ran mozregression-gui again starting with the more narrow range 2019-10-30 to 2019-11-02. Same result as comment 2.

Flags: needinfo?(gingerbread_man)

Shane Caraveo (:mixedpuppy)

Comment 6

•

5 years ago

Luca will be way better than I at triaging a devtools/addon issue.

Flags: needinfo?(mixedpuppy) → needinfo?(lgreco)

Tim Nguyen :ntim

Comment 7

•

5 years ago

I can confirm a local backout of bug 1576915 on top of latest m-c fixes the issue, so the regression range is correct here.

Tim Nguyen :ntim

Comment 8

•

5 years ago

Attached patch backout.diff — Details — Splinter Review

I had to solve some conflicts to do hg backout, so posting the diff here so others won't have to go through it.

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 9

•

5 years ago

Thanks ntim, the backout diff was really helpful.

I took a look to this and it looks like a race:

with the backout patch applied: the React DevTools script is being executed and the script tag is injected before the nsXMLPrettyPrinter::PrettyPrint method had a chance to be executed and start to observe the document (and so nsXMLPrettyPrinter::ContentAppended is never called and the pretty printer shadow root isn't removed)
without the backout patch: the React Devtools script seems to still be executed before nsXMLPrettyPrinter::PrettyPrint, but the script tag seems to be append after nsXMLPrettyPrinter::PrettyPrint has been already executed (and so the appended script tag is triggering nsXMLPrettyPrinter::ContentAppended, as expected, and the pretty printer shadow root is removed from the document)

Another hint that this may be due to a race is that, even with the backout patch applied, the pretty print shadow root is also being removed if the React DevTools extension is reloaded (e.g. by disabling and re-enabling it) while the XML document is already fully loaded and pretty printed (and this is not surprising, because reloading the extension is going to execute its content scripts in the existing tabs, which is going to trigger nsXMLPrettyPrinter::ContentAppended).

And so I also confirm that the regression range seems the right one, but I can't see yet how that change could trigger this change in behavior (and I'm honestly still curious about that).

As Gijs pointed out in Comment 4, the easiest fix is likely to make the React Devtools content script injectGlobalHook.js to not inject the script tag in XML documents (e.g. by checking in the content script if document.contentType is "text/xml" before injecting them in the doc).

Flags: needinfo?(lgreco)

Brian Vaughn

Reporter

Comment 10

•

5 years ago

I'm happy to make this change, by the way. Seems like several other extensions also exhibit this broken behavior, but I think it's reasonable for the extension authors to also make that change.

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 11

•

5 years ago

(In reply to Brian Vaughn from comment #10)

I'm happy to make this change, by the way. Seems like several other extensions also exhibit this broken behavior, but I think it's reasonable for the extension authors to also make that change.

Yeah, that's definitely true: any extension with a content script that injects a tag into an attached XML file is going to trigger the same issue (it doesn't need to be a script tag) and so there are going to be many more other extensions that could prevent the XML pretty printing from working as expected.

I'm not sure (at least not yet) if there is something that we can reasonably do on the Firefox side, mainly because the behavior is actually the expected one and it seems that the extensions were not yet breaking the XML pretty printing mostly because of a bit of "luck"
("not yet breaking" in the most common scenario, because reloading the extension was already going to break the XML pretty printing even before the changes applied by Bug 1576915 on Firefox 72).

If we can't fix the issue on the Firefox side (as it seems), it would be good to send out to the extension developers some communication related to this issue and how to fix it on the extensions side.

Unfortunately the first Firefox version affected is going to be released pretty soon (it should be scheduled for Jan 7), and with the festivities in the middle the extension developer may not actually have the time to fix them before the Firefox release date, but the fix is going to be quite simple and so the sooner they know the sooner the affected extensions can be fixed (if actually needed).

Hey Phil,
What do you think about this?
Would it be possible to send out some communication about this to the extension developers in the next few days?

(Near the end of comment 9 there is a mention to how an extension may prevent the issue from being triggered from its content scripts, but let me know if there are other details that you may need from me)

Flags: needinfo?(philipp)

Brian Vaughn

Reporter

Comment 12

•

5 years ago

I believe PR 17739 should resolve this issue for the React DevTools extension at least.

:Gijs (he/him)

Assignee

Comment 13

•

5 years ago

I'd still be curious to know why the patch triggered this different outcome in the race condition. It sounds like the PiP change made a difference as to when we construct a document and/or run the extension handlers, and we don't fully understand why it did so - ideally we should figure that out.

Separately, I wonder if either we could add an opt-in for add-ons to run content scripts on these pages (but not do so by default) and/or if we could make the XML pretty print code ignore content appended by extensions (as opposed to web controlled content). I think we can detect the difference at some points (but perhaps not in the content observer code we're currently using? I'm not sure off-hand).

Luca, do you have any ideas for either the regression cause and/or the viability of the mitigations I suggested?

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 14

•

5 years ago

(In reply to :Gijs (he/him) from comment #13)

I'd still be curious to know why the patch triggered this different outcome in the race condition. ...
Luca, do you have any ideas for either the regression cause and/or the viability of the mitigations I suggested?

Yeah, I was definitely curious too and I wanted to take a deeper look once I was completely back from my PTO.

I took a deeper look this morning and I'm now pretty sure that Bug 1576915 has regressed this behavior just as a side-effect of being the last entries removed from ppmm's sharedData "ChildSingletonActors" Map.

When we do have "ChildSingletonActors" Map entries (no matter what they actually do) it seems that the promise created by mozIExtensionProcessScript::LoadContentScript is consistently able to run earlier than nsXMLPrettyPrinter::PrettyPrint, because ExtensionPolicyService::CheckContentScripts is going to call a MozDocumentObserver's MozDocumentCallback and that seems enough to trigger an earlier CycleCollectedJSContext::PerformMicroTaskCheckPoint, which does run the LoadContentScript promise immediately.

I'm going to attach a reduced test case which reproduces the issue and also shows that a fake entry in the "ChildSingletonActors" Map is enough to prevent the regression (as the backout patch attached by :ntim also does).

(As a side note: only the content scripts with run_at: "document_start" were able to append elements to an XML document without triggering the removal of the XML pretty printed shadow root, content scripts with run_at: "document_idle" or "document_end" were still going to break the XML pretty printing even before this regression).

Separately, I wonder if either we could add an opt-in for add-ons to run content scripts on these pages (but not do so by default) and/or if we could make the XML pretty print code ignore content appended by extensions (as opposed to web controlled content).

an opt-in mechanisms could be reasonable, at least if (as I guess) most of the extension are currently injecting content scripts in the XML files just as a side effect of "not checking the document type" (whereas a subset of extension may still be interested in doing it on purpose, e.g. an extension that implements an RSS feed reader, or a devtools related to XML files).

The main issue would be likely to decide how to provide the opt-in mechanisms (e.g. should the opt in something that the extension does or it is for the user to decide, like for the private browsing access), and how to deal with cross-browsers compatibility if the opt-in mechanisms has to be exposed through the manifest or the APIs.

I'm not sure if ignoring the content appended by the extensions is a feasible solution (it may be if we only want to cover the elements that the extension may add using its own principal, but it may be trickier if we do want to also cover what the extension may do when it does eval some JS code with the webpage principal, which may be possible if the webpage CSP allows it).

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 15

•

5 years ago

Attached file Bug 1605657 - Add test for extension content scripts running on XML prettyprinted documents. — Details

This patch contains a reduced test case for the regression tracked by Bug 1605657.

The test case registers a small test extension composed by just a content script
that matches "<all_urls>" and inject a tag into the attached page as soon as the
content script is being executed.

After the test XML page has been fully loaded, the test case is asserting that the
shadow root created by the XML pretty printer has not been removed.

The test fails consistently as is, but uncommenting the call to the helper function
named injectFakeChildActor makes it to pass consistently (as it does when the backout
for the bug 1576915 patch).

The injectFakeChildActor set into the ppmm's sharedData "ChildSingletonActors" Map
a fake no-op child actor definition, that is enough to prevent the regression introduced
by Bug 1576915 (which shows that the only reason why Bug 1576915 introduced this regression
is as a side-effect of being the last actors to be removed from there).

When there is a ChildSingletonActors that match "<all_urls>" (it doesn't matter what the
actor does, there could be as well not exist any actual actor, the matcher added to the
MozDocumentObserver is enough):

ActorManagerChild will register a MozDocumentObserver with a set of matchers
(if no ChildSingletonActors exist, the MozDocumentObserver is still created by it does
not contain any matchers):
https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/toolkit/modules/ActorManagerChild.jsm#295-313
Loading the XML file will notify the "initial-document-element-inserted" topic on the
nsObserverService (triggered by nsExpatDriver::HandleStartElement -> ... ->
nsContentSink::NotifyDocElementCreated -> nsObserverService::NotifyObservers -> ...)
ExtensionPolicyService::Observe is being notified of the "initial-document-element-inserted" topic
and it calls ExtensionPolicyService::CheckDocument -> ExtensionPolicyService::CheckContentScripts
In ExtensionPolicyService::CheckContentScripts, we do check if any of the extension has content scripts
that match the XML document and we do call mozIExtensionProcessScript::LoadContentScript (which "schedule
a promise"), and this happen consistently with and without any ChildSingletonActors's matchers:
- https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/toolkit/components/extensions/ExtensionPolicyService.cpp#491-508
- https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/toolkit/components/extensions/mozIExtensionProcessScript.idl#16-17
After checking the content scripts, ExtensionPolicyService::CheckContentScripts does check if any
registered MozDocumentObserver does have any matcher that matches the document
(https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/toolkit/components/extensions/ExtensionPolicyService.cpp#510-522)
- Without any ChildSingletonActors with matchers:
  - ExtensionPolicyService::CheckContentScripts does not call extensions::DocumentObserver::NotifyMatch
    and nsXMLPrettyPrinter::PrettyPrint is consistently executed before the promise created by
    mozIExtensionProcessScript::LoadContentScript (and so when the content script run, nsXMlPrettyPrinter
    does remove the pretty printed shadow root)
- With a ChildSingletonActors with matchers (even the fake one created by the test case):
  - ExtensionPolicyService::CheckContentScripts calls extensions::DocumentObserver::NotifyMatch,
    and this is enough to allow the promise created by mozIExtensionProcessScript::LoadContentScript
    to be consistently executed before nsXMLPrettyPrinter::PrettyPrint had a chance to run,
    with the following stack trace (an extract of it):
    ...
    #46 PromiseJobCallback::Call(char const*)
    #47 PromiseJobRunnable::Run(mozilla::AutoSlowOperation&)
    #48 CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool)
    #49 CycleCollectedJSContext::LeaveMicroTask()
    #50 CallbackObject::CallSetup::~CallSetup()
    #51 MozDocumentCallback::OnNewDocument(...)
    #52 extensions::DocumentObserver::NotifyMatch(...)
    #53 ExtensionPolicyService::CheckContentScripts(...)
    ...

:Gijs (he/him)

Assignee

Comment 16

•

5 years ago

Yegh, microtask checkpoint mess. :-(

Thanks for diving into this, Luca!

When we do have "ChildSingletonActors" Map entries (no matter what they actually do) it seems that the promise created by mozIExtensionProcessScript::LoadContentScript is consistently able to run earlier than nsXMLPrettyPrinter::PrettyPrint, because ExtensionPolicyService::CheckContentScripts is going to call a MozDocumentObserver's MozDocumentCallback and that seems enough to trigger an earlier CycleCollectedJSContext::PerformMicroTaskCheckPoint, which does run the LoadContentScript promise immediately.

I'd be worried that this means there are other cases (ie not just XML pretty printing) where this change will have altered when we run document_start content scripts, which would impact less niche things. It's hard to know for sure because it all depends on the microtask scheduling and when the next point is for those other things - which might be "soon", just not soon enough for the XML case...

Any idea how we'd figure out what else is impacted here, and/or how scary it'd be to "manually" introduce another microtask checkpoint here to keep existing behaviour? Or am I seeing ghosts? If my fears are realistic, may be worth looping in :arai and/or :smaug .

status-firefox72: --- → fix-optional

status-firefox73: --- → affected

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 17

•

5 years ago

(In reply to :Gijs (he/him) from comment #16)

I'd be worried that this means there are other cases (ie not just XML pretty printing) where this change will have altered when we run document_start content scripts, which would impact less niche things. It's hard to know for sure because it all depends on the microtask scheduling and when the next point is for those other things - which might be "soon", just not soon enough for the XML case...

I was mainly worried if this was going to mean that a run_at: "document_start" content script could potentially not be able to get to the document before it is fully loaded, because that would break pretty much the main assumption for the extension content scripts which want to run on document_start
(but it was going to be surprising if we had regressed that without noticing, as I'm pretty sure that we have covered it at least with one explicit test case)

Based on the investigation results that doesn't seem to be the case, and so it seems to be that we may only have affected the ability of the content script of running before other Firefox internals like the XML pretty printing.

Any idea how we'd figure out what else is impacted here, and/or how scary it'd be to "manually" introduce another microtask checkpoint here to keep existing behaviour? Or am I seeing ghosts? If my fears are realistic, may be worth looping in :arai and/or :smaug .

My feeling is that this change to the microtask scheduling shouldn't be breaking any other major thing (the most common assumption that a content script usually does is that it does run on document_start before any webpage script, and that doesn't look to have been regressed, and it doesn't neither look as something it could regress because of that change).

I would definitely feel better about it if I could be 100% sure of it that nothing besides the XML pretty print is being impacted, but I kept thinking to what other things this may have an impact to and I've not been able to think and/or identify any yet.

I'm not really sure how reasonable may be to "manually" introduce another microtask checkpoint just to make sure that the mozIExtensionPorcessScript::LoadContentScript's Promise is scheduled immediately (at a first glance it seems like it would be something that "kind of kills" the purpose of scheduling the content script execution as a promise instead of just synchronously execute the script), nevertheless another opinion about it from someone more knowledgeable then me on the microtask scheduling may still be good (and it may also help us to identify what other interactions we may have regressed as a side effect of that PictureInPicture change).

Flags: needinfo?(lgreco)

Clara Guerrero ( Need Info Brindusa Tot Please)

Comment 18

•

5 years ago

Hi,

I've updated the flags for Nightly version 74.0a1 (2020-01-07) (64-bit) to affected, I was able to reproduce it with Redux Devtools but not React DevTools since after installing the add-on, the button won't appear enabled.

Best,
Clara

status-firefox74: --- → affected

henrik

Comment 19

•

5 years ago

Clara: React DevTools has worked around this issue now, see https://github.com/facebook/react/issues/17620

:Gijs (he/him)

Assignee

Comment 20

•

5 years ago

OK. Based on the other comments, I think we're good to assume (until evidence to the contrary appears, at least) that there's unlikely to be other fallout along the lines of this from the same patch. Thanks again for all the archaeology. In terms of Firefox-side fixes:

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #14)

I'm not sure if ignoring the content appended by the extensions is a feasible solution (it may be if we only want to cover the elements that the extension may add using its own principal, but it may be trickier if we do want to also cover what the extension may do when it does eval some JS code with the webpage principal, which may be possible if the webpage CSP allows it).

I think this may be our best bet though. Having thought about it a bit more, people can and do still write styled styled XML, and I think at the point where we get the document_start event, we might not know whether we will be using the unstyled XML or not. If it is styled XML (ie basically web content), add-ons may still want to do things to it? E.g. http://feeds.bbci.co.uk/news/rss.xml?edition=int - that's some RSS, but it comes with styling info for browsers like us who don't have an RSS reader (anymore).

Seems unlikely to matter for the react case, but the web is a wild place, so who knows for other add-ons.

I think to start with we'd only care about the content appended directly by add-ons. Would that cover the redux / lastpass testcase, or do they use some kind of indirection that means even that doesn't help?

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 21

•

5 years ago

(In reply to :Gijs (he/him) from comment #20)

I think this may be our best bet though. Having thought about it a bit more, people can and do still write styled styled XML, and I think at the point where we get the document_start event, we might not know whether we will be using the unstyled XML or not. If it is styled XML (ie basically web content), add-ons may still want to do things to it? E.g. http://feeds.bbci.co.uk/news/rss.xml?edition=int - that's some RSS, but it comes with styling info for browsers like us who don't have an RSS reader (anymore).

I assume that if an extension injects some tags into a styled XML file like http://feeds.bbci.co.uk/news/rss.xml?edition=int , it would not break the styling of that page (at least if the extension doesn't change something that would breaks the changes that the xslt file provide by the XML file itself is meant to achieve), and in that case the "XML pretty printer" would already be disabling itself because the page does starts to restyle itself, am I right?

That is actually an interesting example, let's say that an rss.xml file isn't already restyling itself as in this case, and that an extension that implements an RSS reader wants to actually detect XML files loaded in a tab that looks like an RSS feed and then apply (from the extension) some styling to make it looks like a webpage (as the bbc rss.xml file already does on its own).

This seems a legitimate use case, and so it seems something we should allow, even if we are going to make the XML pretty printer to ignore automatically content appended by the extension.

Seems unlikely to matter for the react case, but the web is a wild place, so who knows for other add-ons.

I think to start with we'd only care about the content appended directly by add-ons. Would that cover the redux / lastpass testcase, or do they use some kind of indirection that means even that doesn't help?

I haven't checked lastpass yet, but both React Devtools and Redux Devtools do append a script element and then remove it immediately after:

it would be nice if we could detect just this kind of modification done from an extension and let the XML pretty printed shadow root to stay in that case (instead of auto-remove itself), but still leave an extension to restyle the XML completely if that is what the extension want to achieve.

Flags: needinfo?(lgreco)

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox72: fix-optional → wontfix

status-firefox-esr68: --- → unaffected

:Gijs (he/him)

Assignee

Comment 22

•

5 years ago

(sorry for the delay in response, I had a bunch of other fires to put out.)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #21)

(In reply to :Gijs (he/him) from comment #20)

I think this may be our best bet though. Having thought about it a bit more, people can and do still write styled styled XML, and I think at the point where we get the document_start event, we might not know whether we will be using the unstyled XML or not. If it is styled XML (ie basically web content), add-ons may still want to do things to it? E.g. http://feeds.bbci.co.uk/news/rss.xml?edition=int - that's some RSS, but it comes with styling info for browsers like us who don't have an RSS reader (anymore).

I assume that if an extension injects some tags into a styled XML file like http://feeds.bbci.co.uk/news/rss.xml?edition=int , it would not break the styling of that page (at least if the extension doesn't change something that would breaks the changes that the xslt file provide by the XML file itself is meant to achieve), and in that case the "XML pretty printer" would already be disabling itself because the page does starts to restyle itself, am I right?

I believe so, yes - the pretty printer would be disabled because of the page's own styling things.

That is actually an interesting example, let's say that an rss.xml file isn't already restyling itself as in this case, and that an extension that implements an RSS reader wants to actually detect XML files loaded in a tab that looks like an RSS feed and then apply (from the extension) some styling to make it looks like a webpage (as the bbc rss.xml file already does on its own).

This seems a legitimate use case, and so it seems something we should allow, even if we are going to make the XML pretty printer to ignore automatically content appended by the extension.

Yeah, fair point... so ignoring all extension-inserted content wouldn't work...

Seems unlikely to matter for the react case, but the web is a wild place, so who knows for other add-ons.

I think to start with we'd only care about the content appended directly by add-ons. Would that cover the redux / lastpass testcase, or do they use some kind of indirection that means even that doesn't help?

I haven't checked lastpass yet, but both React Devtools and Redux Devtools do append a script element and then remove it immediately after:

https://github.com/facebook/react/blob/e706721490e50d0bd6af2cd933dbf857fd8b61ed/packages/react-devtools-extensions/src/injectGlobalHook.js#L9-L15

https://github.com/zalmoxisus/redux-devtools-extension/blob/94f7e53800f4665bddc9b7438c5cc75cfb4547cc/src/browser/extension/inject/pageScriptWrap.js#L6-L8

it would be nice if we could detect just this kind of modification done from an extension and let the XML pretty printed shadow root to stay in that case (instead of auto-remove itself), but still leave an extension to restyle the XML completely if that is what the extension want to achieve.

I agree. Does the script load actually do anything in these unstyled docs? I'm probably missing something, but for an XML text/xml doc without an (X)HTML doctype, part of me would think that all the nodes are meaningless (no namespace) and so no script is even loaded this way, as a script node doesn't do anything. That also seems to be what happens for me when I "manually" run those snippets in a console on the example from comment #0. Is that correct or am I missing something?

If so, perhaps we would want to ignore extension-inserted content unless it's an XML-stylesheet PI or something. I suppose other add-ons may be using createElementNS to create a "real" HTML script node, which does run script.

In fact, perhaps we could just make the restriction more specific for all content (not just add-on-inserted content)? Apparently, 18 years ago (!) there were a lot of ways to style things, cf the list in https://bugzilla.mozilla.org/show_bug.cgi?id=64945#c113 . But at least some of those have stopped applying (e.g. XBL bindings), and so I wonder if dropping out of the source view only for added html:link nodes, html:style attributes or XML stylesheet PIs would still work? :peterv or :bzbarsky may be better people to ask that if that seems sane to you.

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 23

•

5 years ago

(In reply to :Gijs (he/him) from comment #22)

it would be nice if we could detect just this kind of modification done from an extension and let the XML pretty printed shadow root to stay in that case (instead of auto-remove itself), but still leave an extension to restyle the XML completely if that is what the extension want to achieve.

I agree. Does the script load actually do anything in these unstyled docs? I'm probably missing something, but for an XML text/xml doc without an (X)HTML doctype, part of me would think that all the nodes are meaningless (no namespace) and so no script is even loaded this way, as a script node doesn't do anything. That also seems to be what happens for me when I "manually" run those snippets in a console on the example from comment #0. Is that correct or am I missing something?

it is definitely correct, a "script" element created on a XML document is just another "dummy" XML node, no js code is actually running in the XML page in that case (as we expected).

If so, perhaps we would want to ignore extension-inserted content unless it's an XML-stylesheet PI or something. I suppose other add-ons may be using createElementNS to create a "real" HTML script node, which does run script.

Yep, I can confirm this one too: if the content script creates the script element using createElementNS and the XHTML namespace, it does run js code into the "context" of the XML page as we would expect
(I'm not sure how many addons are currently doing it, but it could be used for a use case like the one we did mention)

In fact, perhaps we could just make the restriction more specific for all content (not just add-on-inserted content)? Apparently, 18 years ago (!) there were a lot of ways to style things, cf the list in https://bugzilla.mozilla.org/show_bug.cgi?id=64945#c113 . But at least some of those have stopped applying (e.g. XBL bindings), and so I wonder if dropping out of the source view only for added html:link nodes, html:style attributes or XML stylesheet PIs would still work? :peterv or :bzbarsky may be better people to ask that if that seems sane to you.

Sure, that seems to make sense to me and it seems that it should allow the extension to implement the "legit use case" without making them able to breaking the XML pretty printed document "by mistake", and so I agree that it may be worth to double-check if it would be doable from a "XML pretty printer" perspective.

Flags: needinfo?(lgreco)

Gingerbread Man

Updated

•

5 years ago

Comment 25

•

5 years ago

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #23)

(In reply to :Gijs (he/him) from comment #22)

In fact, perhaps we could just make the restriction more specific for all content (not just add-on-inserted content)? Apparently, 18 years ago (!) there were a lot of ways to style things, cf the list in https://bugzilla.mozilla.org/show_bug.cgi?id=64945#c113 . But at least some of those have stopped applying (e.g. XBL bindings), and so I wonder if dropping out of the source view only for added html:link nodes, html:style attributes or XML stylesheet PIs would still work? :peterv or :bzbarsky may be better people to ask that if that seems sane to you.

Sure, that seems to make sense to me and it seems that it should allow the extension to implement the "legit use case" without making them able to breaking the XML pretty printed document "by mistake", and so I agree that it may be worth to double-check if it would be doable from a "XML pretty printer" perspective.

Peter/Boris, would an approach like this work?

Flags: needinfo?(peterv)

Flags: needinfo?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Comment 26

•

5 years ago

So the reason we drop prettyprinting on mutation is that the XSLT transformation that uses is one-shot, so if you mutate the DOM the prettyprinted view no longer shows the same thing as the DOM, right? That, plus the "if the page is mutating the DOM, maybe it wants the user to see the result of those mutations" is why we drop prettyprinting on DOM mutation.

In terms of the original bug here, we could just have the prettyprinter do a microtask checkpoint before it starts doing its thing. That does mean the prettyprinting will include the injected bits, of course.

Flags: needinfo?(bzbarsky)

:Gijs (he/him)

Assignee

Comment 27

•

5 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

So the reason we drop prettyprinting on mutation is that the XSLT transformation that uses is one-shot, so if you mutate the DOM the prettyprinted view no longer shows the same thing as the DOM, right? That, plus the "if the page is mutating the DOM, maybe it wants the user to see the result of those mutations" is why we drop prettyprinting on DOM mutation.

Ah, OK, thanks for clarifying.

In terms of the original bug here, we could just have the prettyprinter do a microtask checkpoint before it starts doing its thing. That does mean the prettyprinting will include the injected bits, of course.

That also wfm; in this case the injected bits will be removed from the DOM again anyway.

Flags: needinfo?(peterv)

:Gijs (he/him)

Assignee

Updated

•

5 years ago

Assignee: nobody → gijskruitbosch+bugs

Status: NEW → ASSIGNED

status-firefox73: affected → wontfix

:Gijs (he/him)

Assignee

Comment 28

•

5 years ago

Attached file Bug 1605657 - run a microtask checkpoint before XML pretty printing to avoid webextensions breaking it, r?peterv!,rpl! — Details

Phabricator Automation

Updated

•

5 years ago

Attachment #9118499 - Attachment description: Bug 1605657 - Reduced test case to reproduce the XML pretty print regression → Bug 1605657 - Add test for extension content scripts running on XML prettyprinted documents.

Pulsebot

Comment 29

•

5 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6c933828c43a
run a microtask checkpoint before XML pretty printing to avoid webextensions breaking it, r=peterv,rpl
https://hg.mozilla.org/integration/autoland/rev/77337a5954ad
Add test for extension content scripts running on XML prettyprinted documents. r=Gijs

Razvan Maries

Comment 30

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/6c933828c43a
https://hg.mozilla.org/mozilla-central/rev/77337a5954ad

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

status-firefox75: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla75

Julien Cristau [:jcristau]

Comment 31

•

5 years ago

Is this worth an uplift to beta?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Assignee

Comment 32

•

5 years ago

Comment on attachment 9123623 [details]
Bug 1605657 - run a microtask checkpoint before XML pretty printing to avoid webextensions breaking it, r?peterv!,rpl!

Beta/Release Uplift Approval Request

User impact if declined: Broken XML prettyprinting with some extensions installed
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: Yes
If yes, steps to reproduce: See comment #0
List of other uplifts needed: n/a
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It's early in the nightly cycle and the change here is small, fairly well-understood, and comes with an automated test (thanks Luca!) to check that this is working correctly.
String changes made/needed: nope

Flags: needinfo?(gijskruitbosch+bugs)

Attachment #9123623 - Flags: approval-mozilla-beta?

:Gijs (he/him)

Assignee

Updated

•

5 years ago

Attachment #9118499 - Flags: approval-mozilla-beta?

Flags: qe-verify+

Pascal Chevrel:pascalc

Comment 33

•

5 years ago

Comment on attachment 9123623 [details]
Bug 1605657 - run a microtask checkpoint before XML pretty printing to avoid webextensions breaking it, r?peterv!,rpl!

Low risk and covered by tests, uplift approved for 74.0b3, thanks.

Attachment #9123623 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Pascal Chevrel:pascalc

Updated

•

5 years ago

Attachment #9118499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Arthur Iakab [arthur_iakab]

Comment 34

•

5 years ago

bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/0bbf5deb64fd
https://hg.mozilla.org/releases/mozilla-beta/rev/aa5e67d96ff3

status-firefox74: affected → fixed

Mihai Boldan, Desktop QA [:mboldan]

Updated

•

5 years ago

QA Whiteboard: [qa-triage]

Maria Berlinger [:mberlinger], Services QA

Comment 35

•

5 years ago

Hello,
I’ve managed to reproduce this issue on Firefox 73.0a1 (2019-12-22) using Windows 10x64 by following the STR from Comment 0 but using Redux DevTools add-on instead of React DeveloperTools.
This issue doesn’t occur on Beta and Nightly by using React DeveloperTools add-on.

Although this issue is verified fixed using Redux DevTools add-on on Firefox 74.0b3 and Firefox 75.0a1 (2020-02-14) on the following OSes: Windows 10x64, Windows 7x64, macOS 10.15, Ubuntu 18.04, I want to mention that if you disable and then enable again the Redux DevTools add-on the page is displayed as in the screenshot provided by the reporter with the issue but after refreshing the page is displayed as it should.
Is this an intended behavior?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Assignee

Comment 36

•

5 years ago

(In reply to Maria Berlinger [:maria_berlinger], Release Desktop QA from comment #35)

I want to mention that if you disable and then enable again the Redux DevTools add-on the page is displayed as in the screenshot provided by the reporter with the issue but after refreshing the page is displayed as it should.
Is this an intended behavior?

Yes, that sounds expected. Thanks for clarifying!

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(maria.berlinger)

Maria Berlinger [:mberlinger], Services QA

Comment 37

•

5 years ago

Based on Comment 36, marking this as verified fixed.

Status: RESOLVED → VERIFIED

QA Whiteboard: [qa-triage]

status-firefox74: fixed → verified

status-firefox75: fixed → verified

Flags: qe-verify+

Flags: needinfo?(maria.berlinger)

Maciej Jaros

Comment 38

•

5 years ago

I can confirm that bug 1611423 (LastPass problem) was resolved as well. All works fine in 74.0b4 :-)

Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23)

Comment 39

•

5 years ago

Clearing needinfo assigned to Philipp (we landed a fix in Firefox 75 and uplifted it to Firefox 74, and so the fix for this regression should reach release soon enough, on March 10, it doesn't seem worth to send communications to the extension developer to fix it on their side).

Flags: needinfo?(philipp)

Screen Shot 2019-12-22 at 12.05.47 PM.png 5 years ago Brian Vaughn 184.43 KB, image/png		Details
backout.diff 5 years ago Tim Nguyen :ntim 20.42 KB, patch		Details \| Diff \| Splinter Review
Bug 1605657 - Add test for extension content scripts running on XML prettyprinted documents. 5 years ago Luca Greco [:rpl] [:luca] [:lgreco] (PTO back on July 23) 47 bytes, text/x-phabricator-request	pascalc : approval-mozilla-beta+	Details \| Review
Bug 1605657 - run a microtask checkpoint before XML pretty printing to avoid webextensions breaking it, r?peterv!,rpl! 5 years ago :Gijs (he/him) 47 bytes, text/x-phabricator-request	pascalc : approval-mozilla-beta+	Details \| Review