The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Security
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({helpwanted, regression})

Trunk
x86
Mac OS X
helpwanted, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.13 -
blocking1.8.1.15 -
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testcase-needed][needs branch patch])

Attachments

(6 attachments, 3 obsolete attachments)

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.
Flags: blocking1.9?
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Blocks: 371360
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.

Updated

9 years ago
Summary: Revisit the fix for bug 371360 → Fix for bug 371360 broke sites that use onunload to mask form submissions
Boris, any suggestions on who might be able to help here?  +'ing with P3 priority.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
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.
Assignee: dveditz → 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.
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.
Also in particular it would be great to have some help writing the unit tests for this stuff.
Created attachment 295902 [details] [diff] [review]
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.
Attachment #295902 - Flags: superreview?
Attachment #295902 - Flags: review?
Attachment #295902 - Flags: superreview?(jst)
Attachment #295902 - Flags: superreview?
Attachment #295902 - Flags: review?(jst)
Attachment #295902 - Flags: review?
Summary: Fix for bug 371360 broke sites that use onunload to mask form submissions → [FIX]Fix for bug 371360 broke sites that use onunload to mask form submissions
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12+
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.
Attachment #295902 - Flags: superreview?(jst)
Attachment #295902 - Flags: superreview+
Attachment #295902 - Flags: review?(jst)
Attachment #295902 - Flags: review+
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.
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?
Whiteboard: [testcase-needed]
Duplicate of this bug: 401611

Comment 12

9 years ago
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?
Created attachment 297154 [details]
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.
Martijn, the patch on this bug is not correct; see comment 8.  I'll attach the corrected patch.
Created attachment 297267 [details] [diff] [review]
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.
Attachment #297267 - Flags: superreview?(jst)
Attachment #297267 - Flags: review?(jst)
Comment on attachment 297267 [details] [diff] [review]
Updated to jst's comments and to fix the issue Martijn caught

Interdiff looks good.
Attachment #297267 - Flags: superreview?(jst)
Attachment #297267 - Flags: superreview+
Attachment #297267 - Flags: review?(jst)
Attachment #297267 - Flags: review+
Created attachment 297269 [details] [diff] [review]
Same, but clearer and better comments

This is functionally identical to the previous patch.  Please test this!
Attachment #295902 - Attachment is obsolete: true
Attachment #297267 - Attachment is obsolete: true
With that patch, I'm not even able to start up, it seems.
It stops with the "++DOMWINDOW == 1" console message for me, iirc.
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.
Created attachment 297353 [details] [diff] [review]
Remove bogus '!'

This should do it....
Attachment #297269 - Attachment is obsolete: true
Created attachment 297378 [details]
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.
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.
In IE7, I can't press the back-button, with that testcase. So I think it's replacing the page that's being unloaded.
Created attachment 297418 [details]
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.
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.  :(
Is that a regression though? Did we ever not put stuff in the history? I.e. how does FF2 behave on these "failing" testcases?
> Is that a regression though? Did we ever not put stuff in the history? 

Yes, per comment 21.
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.
Flags: blocking1.8.1.12+ → blocking1.8.1.13?
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.
(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.
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.
Yes, the plan is to convert what I tested into mochitests.

Comment 33

9 years ago
Don't forget to test PageHide as well.
Checked in on trunk.  Martijn, if you can get mochitests together for the comment 4 stuff and this bug, that would be lovely!
Status: NEW → ASSIGNED
Flags: in-testsuite?
Created attachment 299373 [details] [diff] [review]
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.
Created attachment 300122 [details] [diff] [review]
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?
Attachment #300122 - Flags: superreview?
Attachment #300122 - Flags: review?(cbiesinger)
Attachment #300122 - Flags: superreview? → superreview?(jst)
Attachment #300122 - Flags: review?(cbiesinger) → review+

Updated

9 years ago
Attachment #300122 - Flags: superreview?(jst) → superreview+
Checked in the replace() patch.  Marking fixed, and hoping trunk baking goes well...
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Need a branch patch to get this in, and earlier is better. Who's a good assignee for that?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Olli, do you have time to finish the patch in comment 35 for the branch?
We don't have a complete fix for this for the branch yet. Moving to the next release.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
Whiteboard: [testcase-needed] → [testcase-needed][needs branch patch]
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.16?
Flags: blocking1.8.1.15-
Flags: blocking1.8.1.15+
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.
Not blocking the branch on this anymore but still wanted as a regression fix.
Flags: blocking1.8.1.17?
You need to log in before you can comment on or make changes to this bug.