Closed Bug 1348409 Opened 7 years ago Closed 7 years ago

Assertion failure: XRE_IsParentProcess() with window.find

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 - wontfix
firefox53 + fixed
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: jandem, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [fuzzblocker])

Attachments

(1 file, 5 obsolete files)

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.
Mike, can you take a look? Thanks.
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(continuation)
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.
Flags: needinfo?(continuation)
Fair enough. Will take a closer look tomorrow.
Flags: needinfo?(mconley)
Group: core-security → dom-core-security
I haven't forgotten about this - just been bogged down at work weeks. Hopefully time come Monday.
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.
Flags: needinfo?(mconley)
Do you feel comfortable reviewing this, mccr8?
Assignee: nobody → mconley
Attachment #8853153 - Flags: review?(continuation)
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.
Attachment #8853153 - Flags: review?(continuation)
Attached patch bug1348409.diff (obsolete) — Splinter Review
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.
Attachment #8853153 - Attachment is obsolete: true
Attachment #8854006 - Flags: review?(ehsan)
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.
Attachment #8854006 - Flags: review?(ehsan) → review-
(Also it's really hard to tell from this bug what assertion we are hitting!)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8854006 - Attachment is obsolete: true
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).
Attachment #8858008 - Attachment description: diff → Patch
Attached patch Regression tests (obsolete) — Splinter Review
Attachment #8858008 - Attachment is obsolete: true
Comment on attachment 8858008 [details] [diff] [review]
Patch

*sigh*, re-learning how to use bzexport.
Attachment #8858008 - Attachment is obsolete: false
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.
Attachment #8858008 - Attachment is obsolete: true
Attachment #8858010 - Attachment is obsolete: true
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?
Flags: needinfo?(overholt)
Attachment #8858017 - Flags: review?(ehsan)
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.
Flags: needinfo?(overholt)
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.
Attachment #8858017 - Flags: review?(ehsan) → review+
(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!)
Group: dom-core-security
(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. :/
Flags: needinfo?(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.
Flags: needinfo?(overholt)
(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.
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.
Attachment #8858017 - Attachment is obsolete: true
Attachment #8859296 - Flags: review?(ehsan) → review?(mrbkap)
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.
Attachment #8859296 - Flags: review?(mrbkap) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d20f6afea6f0
Stop supporting the showDialog argument for window.find r=mrbkap
[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 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 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.
Attachment #8859296 - Flags: approval-mozilla-beta?
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.
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/1158151c0e29
Backed out changeset d20f6afea6f0 for android failures in test_no_find_showDialog.html a=backout
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e743b48dca21
Stop supporting the showDialog argument for window.find r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/e743b48dca21
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8859296 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Track 54+ as stop supporting the showDialog argument.
(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.)
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.
(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.
Flags: needinfo?(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.
Attachment #8859296 - Flags: approval-mozilla-release?
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.
Attachment #8859296 - Flags: approval-mozilla-esr52?
(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 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
Attachment #8859296 - Flags: approval-mozilla-release? → approval-mozilla-release+
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.
Attachment #8859296 - Flags: approval-mozilla-esr52?
(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.
Flags: qe-verify-
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!
This seems to be triggering quite frequently for us while fuzzing esr52.  Any chance we can get an uplift on this one?
Whiteboard: [fuzzblocker]
ni? Julien for comment #57.
Flags: needinfo?(jcristau)
I'm not sure this meets the bar for ESR to be honest.
Flags: needinfo?(jcristau)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.