Status

SeaMonkey
UI Design
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: mv_van_rantwijk, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed-seamonkey2.0, topcrash})

Trunk
fixed-seamonkey2.0, topcrash
Bug Flags:
blocking-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos?])

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070409 MultiZilla/1.8.3.0m SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a4pre) Gecko/20070420 MultiZilla/1.9.0.0a SeaMonkey/1.5a

Method restoreTab in tabbrowser.xml needs to be patched to fix a security issue.  I have patched MultiZilla little over a week ago now, and it has been confirmed to have fixed the bug i.e. I can write a patch for Seamonkey in case you want.

Note: I have contacted Neil Parkway per e-mail about the bug, and decided to add a bug for it (before he replied to my e-mail).

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

11 years ago
Created attachment 262243 [details]
screenshow displaying the problem
Can you describe the bug, and attach a patch?
(Reporter)

Comment 3

11 years ago
Basically everything after this line:

http://lxr.mozilla.org/seamonkey/source/suite/browser/tabbrowser.xml#1167

in method restoreTab will be skipped when you click cancel in SeaMonkeys postdata confirmation dialog.  The result is a crippled tab: without progress listener and security lock!
(Reporter)

Comment 4

11 years ago
Created attachment 262252 [details] [diff] [review]
cvs diff -u tabbrowser.xml

Proposed patch v1.0

This patch changes the order and to checks nsISHEntry.postData
(Reporter)

Comment 5

11 years ago
Created attachment 262255 [details] [diff] [review]
cvs diff -u tabbrowser.xml

Simplified patch
I don't see anything by that name in the toolkit version of tabbrowser.xml -- am I just missing it under a different name or is this a suite-only feature?
Assignee: guifeatures → neil
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 7

11 years ago
(In reply to comment #6)
> I don't see anything by that name in the toolkit version of tabbrowser.xml --
> am I just missing it under a different name or is this a suite-only feature?

Yes, this is a SeaMonkey suite only feature.
(Assignee)

Comment 8

11 years ago
We have to go back before enabling the filters... that's the whole reason we remove the filters!

Since loading about:blank is a pretty essential part of caching a tab, it would seem that we have three possibilities:
1. Don't allow tabs with postData to be cached
2. Ignore the error and leave the blank page
3. Wait for the postdata cancel error page
(Reporter)

Comment 9

11 years ago
(In reply to comment #8)
> We have to go back before enabling the filters... that's the whole reason we
> remove the filters!
> 
> Since loading about:blank is a pretty essential part of caching a tab, it would
> seem that we have three possibilities:
> 1. Don't allow tabs with postData to be cached
> 2. Ignore the error and leave the blank page
> 3. Wait for the postdata cancel error page

Or check for postData and go back one page?

(Reporter)

Comment 10

11 years ago
Created attachment 262399 [details] [diff] [review]
cvs diff -u tabbrowser.xml

This patch (v1.3) restores tabs with of without post data.
Attachment #262252 - Attachment is obsolete: true
Attachment #262255 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #262399 - Attachment is patch: true
Attachment #262399 - Attachment mime type: text/csv → text/plain
(Reporter)

Comment 11

11 years ago
Created attachment 262403 [details] [diff] [review]
 cvs diff -u tabbrowser.xml

Only reattach the old history before gotoIndex(shIndex - 1) and not before goback()
Attachment #262399 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Not bad, although that assumes that the previous index has no postdata ;-)

Note that <form target="_blank" action="POST"> will create a tab with one index with postdata, so (how) do we deal with that?
(Reporter)

Comment 13

11 years ago
Created attachment 262433 [details] [diff] [review]
cvs diff -u tabbrowser.xml

Something like this?
Attachment #262403 - Attachment is obsolete: true
(Reporter)

Comment 14

11 years ago
Created attachment 262437 [details] [diff] [review]
 cvs diff -u tabbrowser.xml

The previous patch was really untested, typed out of the blue, but this one seems to work.  What do you think of this one Neil?
Attachment #262433 - Attachment is obsolete: true
(Reporter)

Comment 15

11 years ago
Created attachment 262439 [details] [diff] [review]
cvs diff -u tabbrowser.xml

Err. That should have been this one (:
Attachment #262437 - Attachment is obsolete: true
(Reporter)

Comment 16

11 years ago
Created attachment 262441 [details] [diff] [review]
cvs diff -u tabbrowser.xml

This patch addresses Neils point about the previous index having postdata, or not.
Attachment #262439 - Attachment is obsolete: true
I think we should back out the feature.  It exposes a gecko crash nobody cares about / knows how to fix.
(Reporter)

Comment 18

11 years ago
(In reply to comment #17)
> I think we should back out the feature.  It exposes a gecko crash nobody cares
> about / knows how to fix.

Oh?  I personally haven't seen any crashes so far, so how/when exactly does it crash?  Is there a a bug report files for this crash?  
(In reply to comment #18)
> (In reply to comment #17)
> > I think we should back out the feature.  It exposes a gecko crash nobody cares
> > about / knows how to fix.
> 
> Oh?  I personally haven't seen any crashes so far, so how/when exactly does it
> crash?  Is there a a bug report files for this crash?  
> 

http://talkback-public.mozilla.org/reports/mozilla/ click "MozillaTrunk" - it's our #10 crasher.

Comment 20

11 years ago
sam/ctho, any crash data still around for this?  should this be nomniated for 
Keywords: topcrash
Whiteboard: sg:?

Comment 21

11 years ago
any upcoming seamokey release?
Whiteboard: sg:? → [sg:?]
KaiRo would know better than I. I'm not clear what the crash was since SeaMonkey no longer uses Talkback and the data wasn't pasted in this bug.

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design

Comment 23

9 years ago
The crash was fixed some time ago. Do we need to move along with this, if so, who can drive that?

Comment 24

9 years ago
Neil, do we need to do something about this?
Flags: blocking-seamonkey2?
(Assignee)

Comment 25

9 years ago
Firefox now has undo close tab too so it would be interesting to compare whether it suffers from a similar problem and how they deal with it.
I can't find a a concise description of what this bug report actually covers, but it appears to be related to the fact that SeaMonkey keeps actual documents around for undo. Firefox's undo close tab uses session restore, which serializes all the needed data to JSON, so I don't think this problem is relevant there.
(Assignee)

Comment 27

9 years ago
OK, so here's the concise description:

How do you restore a tab that contains an uncached postdata response?
I think we serialize the postData that was sent, and resend it when reopening:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1182

Simon knows the details better than I, though.
(Assignee)

Comment 30

9 years ago
So, this is not usually a problem with Firefox because a) it defaults to not saving posted pages (which I guess we could fake by clearing the post data on the existing history entry) b) it defaults to not saving https data at all (which reduces the chance of it running in to an uncached page).

Comment 31

9 years ago
(In reply to comment #30)
> So, this is not usually a problem with Firefox

This issue doesn't exist in Firefox at all because we first create an empty tab (through tabbrowser.xml's API) and only afterwards try to restore as much of its state as possible (in nsSessionStore.js).

(In reply to comment #29)
> Looks like it just eats the exception.

... at which point the worst that could happen is that the Back/Forward buttons aren't properly enabled.
(Assignee)

Comment 32

9 years ago
I'm going to work around this by copying the sessionstore behaviour, which is to GET the page if it's no longer cached.
(Assignee)

Comment 33

9 years ago
Created attachment 399274 [details] [diff] [review]
Proposed patch
Attachment #399274 - Flags: review?(iann_bugzilla)

Updated

9 years ago
Attachment #399274 - Flags: review?(iann_bugzilla) → review+

Updated

9 years ago
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
(Assignee)

Comment 34

9 years ago
Pushed changeset 92ca5d53ce94 to comm-central.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Attachment #399274 - Flags: approval-seamonkey2.0+

Updated

9 years ago
Keywords: fixed-seamonkey2.0

Comment 35

9 years ago
Neil, can that fix be applied to 1.x. source for an eventual 1.1.19 release?

Comment 36

9 years ago
Sorry for commenting twice for this, the other question is if/when we should open up this report to the public.
I don't think there's an exploitable vulnerability here. It breaks our security indicators but not in a way that's useful to an attacker
Group: core-security
Whiteboard: [sg:?] → [sg:dos?]
You need to log in before you can comment on or make changes to this bug.