1348409 - Assertion failure: XRE_IsParentProcess() with window.find

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan de Mooij [:jandem]]

Passing true as the showDialog argument to window.find will assert with e10s:

<script>
window.find("foo", false, false, false, false, false, true);
</script>

Stack looks like this:

nsWindowWatcher::OpenWindowInternal
nsWindowWatcher::OpenWindow2
nsGlobalWindow::OpenInternal
nsGlobalWindow::OpenDialog
nsGlobalWindow::FindOuter
nsGlobalWindow::Find

I wrote a tiny DOM fuzzer and it hit this pretty quickly so it's probably a known issue? Filing s-s just in case this is bad.

Comment 1

[Andrew McCreight [:mccr8]]

Mike, can you take a look? Thanks.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Until this day, I'd never even heard of this API.

It looks like it was added wayyyyy back in the day (bug 9550)

It's also non-standard: https://developer.mozilla.org/en-US/docs/Web/API/Window/find

What's happening here is that nsGlobalWindow::Find is attempting to open a dialog, and that in e10s mode, dialog windows aren't supported. This straight up will not work.

I also just tried to make this work in non-e10s mode, and I can't even get the dialog to appear. Is this thing well tested?

Bug 672395 seems to be all about killing window.find... is it worth attempting to add e10s support here, or should we just disable this thing once and for all?

Comment 3

[Andrew McCreight [:mccr8]]

I wouldn't bother trying to make it work. I just want to be sure it doesn't do anything bad in a content process.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Fair enough. Will take a closer look tomorrow.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I haven't forgotten about this - just been bogged down at work weeks. Hopefully time come Monday.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

The reason we're entering the dialog stuff is because window.find apparently accepts an argument that will spawn a Find dialog when calling window.find.

That dialog is not well supported. I've just tried Edge, IE10, Chrome 57 and Safari 10.1, and none of them seem to support it.

The dialog is actually broken in non-e10s mode on Firefox 53. If mozregression (and my regression testing) is to be believed, that happened with one of these two commits: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8

We're asserting there for sanity, and I can't immediately identify anything dangerous here, but I think we should just straight-up remove the dialog feature support when in the content process.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Don't try to show dialogs for window.find from the content process

Do you feel comfortable reviewing this, mccr8?

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8853153 [details] [diff] [review]
Don't try to show dialogs for window.find from the content process

Review of attachment 8853153 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should get a real DOM peer to review this, or at least to ok my reviewing it.

Looking at the code, I think you have to test for (aString.IsEmpty() || aShowDialog) before you bail out, because we hit the weird dialog code if either of those things is true. I could be wrong.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: bug1348409.diff

Only if you have time, ehsan. If you don't, let me know, and I'll redirect.

Summary:

A rarely used DOM feature (window.find with dialogs) is broken with e10s, and causes us to fail an assertion in debug builds. I've traced through the code, and I don't _believe_ that it can be used for any kind of privilege escalation, but this patch is of the "better safe than sorry" variety.

I'd like to land this, and then file a new bug to just remove the dialog feature entirely from Gecko.

Comment 10

[(no longer active)]

Comment on attachment 8854006 [details] [diff] [review]
bug1348409.diff

OK, so I guess we never really supported the showDialog argument in e10s, fine.  But throwing is rather harsh, the caller didn't really pass anything invalid.  You should return false, and not throw.  You should also add a test for this.

I checked the Chromium source code and they don't support showDialog: <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp?l=898&rcl=a1746a908f94b40aea3c37d63734d4667e3835bc>.  Webkit has https://bugs.webkit.org/show_bug.cgi?id=13016 with no activity.  In bug 672395 there is general consensus that killing showDialog would be a good thing but that needs to go through an intent to unship process in a separate bug.

Comment 11

[(no longer active)]

(Also it's really hard to tell from this bug what assertion we are hitting!)

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8858008 [details] [diff] [review]
Patch

Okay, this patch causes us to return false early (instead of throwing) if the page attempts to request a find dialog in the content process.

Note that in non-debug builds, we weren't asserting - we were throwing an NS_ERROR_FAILURE though, because a nsIWindowCreator wasn't being found by WindowWatcher for the content process (naturally).

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression tests

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8858008 [details] [diff] [review]
Patch

*sigh*, re-learning how to use bzexport.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

So, for clarity, we're asserting here:

http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/components/windowwatcher/nsWindowWatcher.cpp#778

because something in the content process is requesting a "dialog" type of window.

Note that we'll return NS_ERROR_FAILURE in this window.find case anyways, since we'll get to here: http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/components/windowwatcher/nsWindowWatcher.cpp#958-961, set rv to NS_ERROR_FAILURE, and then fail to find an nsIWindowCreator.

So we end up returning that NS_ERROR_FAILURE here: http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1027-1029

The window features that nsGlobalWindow::FindOuter requests from the nsWindowWatcher are statically defined as "chrome, resizable=no, dependent=yes", and the URI is also statically defined:

http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/base/nsGlobalWindow.cpp#10128-10131

So I can't see us falling through to another result except throwing NS_ERROR_FAILURE in this case, and we don't set any state. So I don't think this is any threat.

In any case, having toyed with those more, I agree with ehsan - we shouldn't throw, we should return false in the event that the showDialog argument is passed.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be8360f9783e

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa6dacacb46

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Remove showDialog support from window.find

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8858017 [details] [diff] [review]
Remove showDialog support from window.find

I know you're backlogged like crazy, ehsan. Let me know if you want me to redirect this.

Hey overholt: it looks like window.find with showDialog has been busted for e10s since practically forever, and we're about to break it for non-e10s in Firefox 53 due to an unrelated change (see comment 6). My feeling is that we should just axe showDialog from window.find from Gecko in general, and be consistent across both e10s and non-e10s here.

What's the procedure to make that official? What kind of mail do I need to send and where?

Comment 21

[Andrew Overholt [:overholt]]

We don't have a strict process but I'd recommend sending an "Intent to unship" email loosely following the template here: https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Email_templates. Send to dev-platform and I guess firefox-dev. I'd like to get a wide audience seeing this. Kohei will likely see it and update the compatibility site without you having to explicitly ask him. Once you've got rough consensus (and haven't been Warnocked) on the lists, tell release-drivers. I'm happy to help if you need any more information or pointers.

Comment 22

[(no longer active)]

Comment on attachment 8858017 [details] [diff] [review]
Remove showDialog support from window.find

Review of attachment 8858017 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below.

::: dom/base/nsGlobalWindow.cpp
@@ -10109,4 @@
>    if (aString.IsEmpty() || aShowDialog) {
> -    // See if the find dialog is already up using nsIWindowMediator
> -    nsCOMPtr<nsIWindowMediator> windowMediator =
> -      do_GetService(NS_WINDOWMEDIATOR_CONTRACTID);

Ugh, sorry I think I wasn't clear enough.  I was suggesting that we should return false *only for e10s*.  This patch is effectively unshipping the interactive find API for non-e10s.  Not that I'm objecting to doing that per se, but the right way to do that isn't in a closed bug without the intent to unship process.

But since we have never supported this for e10s, it's OK to replace the exception that we currently throw with the false return value which we arguably should have always be returning here.

In order to avoid another review cycle, please return early here if you're in the content process. :-)

::: dom/tests/mochitest/bugs/mochitest.ini
@@ +161,5 @@
>  [test_bug1022869.html]
>  [test_bug1112040.html]
>  [test_bug1160342_marquee.html]
>  [test_bug1171215.html]
> +[test_no_find_showDialog.html]

And make this test e10s only for now.

Comment 23

[(no longer active)]

(In reply to Andrew Overholt [:overholt] from comment #21)
> We don't have a strict process but I'd recommend sending an "Intent to
> unship" email loosely following the template here:
> https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Email_templates. Send to
> dev-platform and I guess firefox-dev. I'd like to get a wide audience seeing
> this. Kohei will likely see it and update the compatibility site without you
> having to explicitly ask him. Once you've got rough consensus (and haven't
> been Warnocked) on the lists, tell release-drivers. I'm happy to help if you
> need any more information or pointers.

Please note that as I mentioned in comment 10, this needs to be done in the context of bug 672395, I think.

Also, I really think this bug should be opened up.  There is nothing security sensitive here given the great analysis in comment 16 (thanks for doing that!)

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #22)
> Comment on attachment 8858017 [details] [diff] [review]
> Remove showDialog support from window.find
> 
> Review of attachment 8858017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below.
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ -10109,4 @@
> >    if (aString.IsEmpty() || aShowDialog) {
> > -    // See if the find dialog is already up using nsIWindowMediator
> > -    nsCOMPtr<nsIWindowMediator> windowMediator =
> > -      do_GetService(NS_WINDOWMEDIATOR_CONTRACTID);
> 
> Ugh, sorry I think I wasn't clear enough.  I was suggesting that we should
> return false *only for e10s*.  This patch is effectively unshipping the
> interactive find API for non-e10s.  Not that I'm objecting to doing that per
> se, but the right way to do that isn't in a closed bug without the intent to
> unship process.

Keep in mind that while evaluating this bug, I noticed that we're about to break window.find with showDialog on non-e10s in 53. See comment 6 - something in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8 seems to have busted it (cc'ing ckerschb now about that).

If this thing is going to be broken anyways when 53 goes out the door (in only a few days!), e10s or non-e10s regardless, I suspect we should just do it more deliberately, and return false, rather than have the showDialog fail due to some other inner workings of Gecko.

Hey overholt, is there anything I need to do to let folks know that window.find with showDialog is going to be broken for _all_ users in 53? I don't think that part of comment 6 got noticed. :/

Comment 25

[Andrew Overholt [:overholt]]

(In reply to Mike Conley (:mconley) from comment #24)
> Hey overholt, is there anything I need to do to let folks know that
> window.find with showDialog is going to be broken for _all_ users in 53? I
> don't think that part of comment 6 got noticed. :/

I'd still recommend emailing lists and telling release-drivers. It's also worth asking someone like Mike Taylor who has lots of webdev connections.

The person who wrote https://bugzilla.mozilla.org/show_bug.cgi?id=672395#c36 isn't going to be happy.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrew Overholt [:overholt] from comment #25)
> I'd still recommend emailing lists and telling release-drivers. It's also
> worth asking someone like Mike Taylor who has lots of webdev connections.

Thanks, will craft a thing.

> The person who wrote https://bugzilla.mozilla.org/show_bug.cgi?id=672395#c36
> isn't going to be happy.

True, assuming that they were using the showDialog feature. If they weren't, the behaviour should not change.

Comment 27

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

Tracking this for 53; we'll need figure out if this is something we can disable in a system addon post-release or with a dot release.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Mail sent to dev-platform, release-drivers: https://groups.google.com/forum/#!topic/mozilla.dev.platform/gn4364N4TlY

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1348409 - Stop supporting the showDialog argument for window.find

The dialog functionality of the non-standard window.find API has been broken
with e10s since it shipped, and bug 1182569 or bug 1232432 (or both) have
broken it for non-e10s.

This patch remove showDialog support entirely, for both e10s and non-e10s,
in a more deliberate way. We now ignore the argument.

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

Comment 30

[Kohei Yoshino]

Posted the site compatibility note, just in case: https://www.fxsitecompat.com/en-CA/docs/2017/window-find-dialog-support-has-been-dropped/

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

https://reviewboard.mozilla.org/r/131306/#review134606

Let's do this. I wonder if any platforms warn for unused arguments. If so, adding `Unused << aShowDialog;` might not be the worst thing in the world to add somewhere to the function.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131306/diff/1-2/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d20f6afea6f0
Stop supporting the showDialog argument for window.find r=mrbkap

Comment 34

[Mike Conley (:mconley) (:⚙️)]

[Tracking Requested - why for this release]:

See the note I sent to dev-platform and release-drivers in comment 28.

Requesting tracking for 54 because we probably want window.find's showDialog broken more intentionally (in that the dialog argument will be ignored and that the find can continue) rather than the current status quo, where we throw an exception.

For ESR 52, the show dialog feature throws an exception in the e10s case, but works in the non-e10s case. Not sure if we want to do anything there, but requesting tracking anyways.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

https://reviewboard.mozilla.org/r/131306/#review134606

> If so, adding Unused << aShowDialog; might not be the worst thing in the world to add somewhere to the function.

Good idea. Done.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Approval Request Comment
[Feature/Bug causing the regression]:

e10s, and either bug 1182569 or bug 1232432 for the non-e10s case.

[User impact if declined]:

Users visiting web pages that use window.find with the dialog functionality might find that the page doesn't behave properly (since it might throw an exception instead of ignoring the dialog argument).

[Is this code covered by automated tests?]:

Yes.

[Has the fix been verified in Nightly?]:

Not yet, just landed on autoland.

[Needs manual test from QE? If yes, steps to reproduce]: 

No, I don't think.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No, I don't think so.

[Why is the change risky/not risky?]:

We're avoiding entering a codepath that would attempt to open a find dialog. We've just removed that codepath entirely. This is pretty self-contained.

[String changes made/needed]:

None.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

I don't think I'm going to request uplift for ESR 52. My rationale being that if there really are users on that channel depending on the dialog, we have a greater probability that it'll still work for them because there's a chance they're running in non-e10s mode.

For the users with e10s on ESR 52... well, we're throwing. And that's not great, but I'm not sure it warrants an uplift, as this isn't security related.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This broke some android mochitests: https://treeherder.mozilla.org/logviewer.html#?job_id=93059736&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/732dd6295021

Comment 39

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1158151c0e29
Backed out changeset d20f6afea6f0 for android failures in test_no_find_showDialog.html a=backout

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131306/diff/2-3/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e743b48dca21
Stop supporting the showDialog argument for window.find r=mrbkap

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/e743b48dca21

Comment 43

[Gerry Chang [:gchang]]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Remove showDialog support for both e10s and non-e10s. Beta54+. Should be in 54 beta 2.

Comment 44

[Gerry Chang [:gchang]]

Track 54+ as stop supporting the showDialog argument.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a236801aa022

Comment 46

[(no longer active)]

(In reply to Mike Conley (:mconley) from comment #26)
> > The person who wrote https://bugzilla.mozilla.org/show_bug.cgi?id=672395#c36
> > isn't going to be happy.
> 
> True, assuming that they were using the showDialog feature. If they weren't,
> the behaviour should not change.

That comment is about the non-interactive use of the API (aka not what this bug is discussing.)

Comment 47

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

Mike, do you think we should uplift this to 53? Does it warrant a dot release ? So far, we don't have another clear driver for 53 desktop.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #47)
> Mike, do you think we should uplift this to 53? Does it warrant a dot
> release ? So far, we don't have another clear driver for 53 desktop.

I wouldn't roll a dot release _just_ for this bug, but if you're already doing one, this would be a fine, safe ride-along.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

See comment 36 for the approval request form.

Pre-emptively requesting approval for release, in the event that we want this patch as a ride-along to a dot release on 53.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

I have to believe we'll want this on ESR52 as well under the circumstances.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #50)
> I have to believe we'll want this on ESR52 as well under the circumstances.

Reason being that ESR52 is shipping e10s to a large number of people. Also just feels like the right thing to do from a consistency standpoint.

Comment 52

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

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Since we broke some things about finding in a page for non-e10s users in 53, let's uplift this now to m-r as a ridealong for 53.0.2

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8859296 [details]
Bug 1348409 - Stop supporting the showDialog argument for window.find

Per IRC discussion, going to punt on ESR52 for now unless we start getting complaints from enterprise users about it.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6850ab42fdb7

Comment 55

[Andrei Vaida [:avaida]]

(In reply to Mike Conley (:mconley) from comment #36)
> [Is this code covered by automated tests?]:
> 
> Yes.
> 
> [Has the fix been verified in Nightly?]:
> 
> Not yet, just landed on autoland.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> No, I don't think.

Setting qe-verify- based on Mike's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 56

[Chris Mills [:cmills]]

I have removed the aShowDialog parameter from the find() page, and added a note to the Fx55 release notes:

https://developer.mozilla.org/en-US/docs/Web/API/Window/find
https://developer.mozilla.org/en-US/Firefox/Releases/55#Removals_from_the_web_platform

Let me know if that looks OK. Thanks!

Comment 57

[Jason Kratzer [:jkratzer]]

This seems to be triggering quite frequently for us while fuzzing esr52.  Any chance we can get an uplift on this one?

Comment 58

[Mike Taylor [:miketaylr]]

ni? Julien for comment #57.

Comment 59

[Julien Cristau [:jcristau]]

I'm not sure this meets the bar for ESR to be honest.