409888 - [FIX]Fix for bug 371360 broke sites that use onunload to mask form submissions

Status: RESOLVED FIXED

(Core :: Security, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Apparently the fix for bug 371360 broke some sites (on branch, too).  See bug 401611 and bug 371360 comment 53.

There's a proposed fix design in bug 371360 comment 65, but I very much doubt I'll be able to write and test the fix in time for 1.8.1.12, or for that matter in time for 1.9.

Help here would be very much appreciated.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The additional fun is that we'd have to allow OnLinkClick during unload if the site we're going to is the same as the link click target (so form.submit will work), but not otherwise.  Then when the link click event fires, we'll want to check that the currently-loaded page is still the same as the page that was loading when the event got posted, so we don't leak information through the referrer header.

Comment 2

[Damon Sicore (:damons)]

Boris, any suggestions on who might be able to help here?  +'ing with P3 priority.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bz: I think we really need your help on this one. Me and jst talked this over
and it's a bit hard to know exactly what the right thing to do here as we don't
fully understand all the issues involved. Would be great if you could grab me
in irc and we can talk this one over.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sicking and I talked this over, and it looks like it might not be as bad as I was afraid it would be.  I'll get a patch up, but I'd love some help with testing.  Specifically:

1) Verifying that all the bugs listed as fixed in bug 371360 comment 23 are
   still fixed
2) Verifying that bug 401611 is fixed
3) Verifying that the situation described in bug 371360 comment 53 is fixed

Damon, if you can get someone to help with those that would be wonderful.

Comment 5

[Boris Zbarsky [:bzbarsky]]

In particular, when testing bug 251944 we should make sure that setting a timeout in the unload handler and doing the load from the timeout doesn't allow people to work around the fix for that bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Also in particular it would be great to have some help writing the unit tests for this stuff.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch

I'd like to get this in on trunk and tested (in some order); if it works as desired the backport to branch is not too bad, except for the async event stuff needing a bit of rewriting.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 295902 [details] [diff] [review]
Trunk patch

- In nsDocShell::InternalLoad():

+    if (mFiredUnloadEvent && IsOKToLoadURI(aURI)) {
+        // Do this asynchronously
+        nsCOMPtr<nsIRunnable> ev =
+            new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
+                                  aWindowTarget, aTypeHint,
+                                  aPostData, aHeadersData, aLoadType,
+                                  aSHEntry, aFirstParty);
+        return NS_DispatchToCurrentThread(ev);
+    }

This needs to bail if (mFiredUnloadEvent && !IsOkToLoadURI()), as we talked about on irc.

Also, I'll note here that if this is called from an unload handler and the handler opens up a modal dialog we'll run the runnable only to get back here while we're still in the unload handler, so we'll post another runnable, etc etc. I don't *think* that's much of a problem, short of maybe pegging the cpu while the modal dialog is open, and it would probably make the result inconsistent if you had code that did something like this:

  location = foo.com;
  location = bar.com;
  alert();

In that case I doubt the result would be consistent. I also doubt it's something that people will run into, so I don't really think it matters...

r+sr=jst with the first issue fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The test to catch the issue jst found there is a location set from unload that sets the location to some URI at a different server from the URI being loaded.  Such a set should be a no-op, but with this patch it probably actually loads the URI.

Comment 10

[Samuel Sidler (old account; do not CC)]

Tim, we'd really like to get this both on trunk and on branch, however we need tests before it can land. Do you know who would be best to create tests for this?

Comment 12

[Tim Riley [:timr]]

clint (or bc or martijn):  we need some urgent test cases for branch and trunk here. Have you created any tests for OnLinkClick before.  Can you help?

Comment 13

[Martijn Wargers (dead)]

Attachment: 2 testcases

These are 2 testcases. In both cases, the iframe background-color needs to stay/become green.
The first case is what is currently broken on trunk and branch.
Boris, I tried your patch in current trunk build, but with this testcase, the iframe still stays red. It should have become green, no? (or is my testcase flawed?  In IE7 at least, it becomes green)
The patch seems to fix bug 401611, though.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Martijn, the patch on this bug is not correct; see comment 8.  I'll attach the corrected patch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to jst's comments and to fix the issue Martijn caught

jst, could you take a look at this?  Just the diff from the previous version will do; the changes are in LoadHistoryEntry and LoadURI, in addition to the change we discussed on irc.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 297267 [details] [diff] [review]
Updated to jst's comments and to fix the issue Martijn caught

Interdiff looks good.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but clearer and better comments

This is functionally identical to the previous patch.  Please test this!

Comment 18

[Martijn Wargers (dead)]

With that patch, I'm not even able to start up, it seems.
It stops with the "++DOMWINDOW == 1" console message for me, iirc.

Comment 19

[Martijn Wargers (dead)]

Reverting this part of the patch:
-    if (!IsNavigationAllowed()) {
+    // Note: we allow loads to get through here even if mFiredUnloadEvent is
+    // true; that case will get handled in LoadInternal or LoadHistoryEntry.
+    if (!IsPrintingOrPP()) {
makes the problem go away.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove bogus '!'

This should do it....

Comment 21

[Martijn Wargers (dead)]

Attachment: testcase3

You need to click on the link to do the test (first unzip it and then open 409888.htm).
Boris, your latest patch fixes this case, but in this case still a history entry is created, so the back button becomes active. This wasn't a problem on branch and trunk before bug 371360 was fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  How does IE behave on that testcase?  Is it replacing the page being unloaded, or the newly-loading page?

I would have thought we'd replaced the newly-loading page, but maybe I was wrong.  I don't think I can think of a sane way to replace the page being unloaded.

Comment 23

[Martijn Wargers (dead)]

In IE7, I can't press the back-button, with that testcase. So I think it's replacing the page that's being unloaded.

Comment 24

[Martijn Wargers (dead)]

Attachment: testcase4

I guess this is related, also scripts seem to be executed on the newly-loading page. This also seems to have regressed when the patch for bug 371360 landed on trunk and branch.
IE7 doesn't seem to execute scripts on the newly-loading page.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Right.  It sounds like IE bails out of the load if another load is started from unload.  We can try to do that.  It'd likely take more surgery (and more importantly pre-surgical investigation) than I'll have time for for months, and probably not be branch-safe.

Depending on how much compat we want, we may want to back out bug 371360 on the branch and try for an alternate fix.    Someone needs to make a call on this.  I won't have time to work on that either....

To be completely honest, at this point my outstanding reviews for beta3 blockers exceed the total amount of time I have to spend on Mozilla things before beta3 and the next branch release.  So I doubt I'll be able to get much more done here.  :(

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is that a regression though? Did we ever not put stuff in the history? I.e. how does FF2 behave on these "failing" testcases?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Is that a regression though? Did we ever not put stuff in the history? 

Yes, per comment 21.

Comment 28

[Daniel Veditz [:dveditz]]

despite optimism in comment 4 and later, it appears that comment 0 was correct that this would not be able to make 1.8.1.12. We'll check next time to see if we've gotten a tested trunk fix by then.

Comment 29

[Boris Zbarsky [:bzbarsky]]

We sort of need to make a call as to how much we care about replace() replacing the newly-loading page instead of the old one.  If we're OK with that, we can take this patch as soon as we have the tests in hand...

Personally, I'm OK with that.  It's less severe breakage than what we have now, for sure.

Comment 30

[Martijn Wargers (dead)]

(In reply to comment #29)
> We sort of need to make a call as to how much we care about replace() replacing
> the newly-loading page instead of the old one.  If we're OK with that, we can
> take this patch as soon as we have the tests in hand...

Boris, what kind of tests do you still need? I'm not really sure what to test anymore. I also tested the things in comment 4 with your patch.

Comment 31

[Boris Zbarsky [:bzbarsky]]

If you tested the stuff in comment 4, and it all works, that's great.  I didn't realize that.

Ideally we'd get unit tests for that into the tree, but that shouldn't block this patch landing.  Would you mind converting the tests you attached to this bug into reftests or mochitests?  I'll work on getting this landed on trunk so we can get some feedback ASAP.

Comment 32

[Martijn Wargers (dead)]

Yes, the plan is to convert what I tested into mochitests.

Comment 33

[Bob Clary [:bc] (inactive)]

Don't forget to test PageHide as well.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  Martijn, if you can get mochitests together for the comment 4 stuff and this bug, that would be lovely!

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial branch merge

This still needs the event stuff rewritten to use PLEvent.  I'm not sure when I'll have a chance to do that...

I talked to Samuel and Daniel, and the plan is to skip this for 1.8.1.12, but try to get it in early in the next cycle.  If someone can pick up the patch and get it working against branch, that would be very helpful; I'm not likely to be able to do it in the timeframe involved.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: Make replace() work as it used to

This is scary stuff.  The basic issue was that the replace() coming in the middle of SetupNewViewer() changed mLoadType right there and then (in InternalLoad).  As a result, the load coming in that we were setting up the viewer for got treated as a replace load!  So did the new load coming in on top of that.  So we ended up replacing the current page's SHEntry, twice.

This code mimics that behavior.  I moved the check down lower to come after the various security checks, especially CheckLoadingPermissions, since that check relies on the right JSContext being on the stack, which will no longer be the case when our event fires.  That also ensures that we only do the replace thing for replace loads that will actually happen.

Changing the type of the existing load to replace no matter what the original type was is a little wacky (e.g. it might affect history loads... but so did the old code).  If we want, we could restrict that clause to normal-ish loads of various sorts (that is, only clobber mLoadType if it's already LOAD_TYPE_NORMAL, or NORMAL_REPLACE, or LINK).

Martijn, would you be willing to give this a spin?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Checked in the replace() patch.  Marking fixed, and hoping trunk baking goes well...

Comment 38

[Daniel Veditz [:dveditz]]

Need a branch patch to get this in, and earlier is better. Who's a good assignee for that?

Comment 39

[Samuel Sidler (old account; do not CC)]

Olli, do you have time to finish the patch in comment 35 for the branch?

Comment 40

[Samuel Sidler (old account; do not CC)]

We don't have a complete fix for this for the branch yet. Moving to the next release.

Comment 41

[Daniel Veditz [:dveditz]]

Apparently we're lacking sufficient testcase coverage to take this safely on branch, will have to wait and hope for better when Boris gets back in July. Although if it's been broken since Firefox 2.0.0.8 maybe sites stopped using this technique? One can hope.

Comment 42

[Boris Zbarsky [:bzbarsky]]

I should note that at this point my "try to do this when I get back in July" list has 100+ items on it.  So I'd _still_ love it if someone comes through and writes the tests for this... might let me focus on something that has more of a barrier to entry instead.

Comment 43

[Daniel Veditz [:dveditz]]

Not blocking the branch on this anymore but still wanted as a regression fix.