135825 - No scrollbars in P3P summary window

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

1. Go to www.msn.com (or any other site supporting P3P).
2. Open Page Info dialog, and go to Privacy tab.
3. Hit Summary button.

Expected results: new window with scrollbars

Actual results: no scrollbars present

If you give focus to the window you can scroll with up/down keys.

You need the patch from bug 124503 to test this.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Based on a patch by jst for bug 55334 to workaround bug 78070. You'll also need
attachment 79249 [details] [diff] [review] from bug 56087.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Testcase - source

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Testcase - style

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Testcase - one-click, transforms into the iframe

Comment 5

[Axel Hecht]

The nsCOMPtr root conflicts with attachement 79249.

I get the following assertion after scrolling a bit:

###!!! ASSERTION: different documents: 'oldAncestor == newAncestor', file
/tmp/mozilla/content/events/src/nsEventStateManager.cpp, line 3474
###!!! Break: at file /tmp/mozilla/content/events/src/nsEventStateManager.cpp,
line 3474

jst says he doesn't know that one from his workaround, could someone ensure
me that this ain't evil?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I saw that assertion, without peterv's patch, on regular HTML page going
back/forward in history.

I pulled & built today, applied peterv's patch (I needed to remove one line as
patch did not work 100% correct, hopefully the patch was still ok... would be
good to attach a new patch just in case), and I did not see any assertions using
the testcase. 

I have not yet tested with P3P.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 79253 [details] [diff] [review]
v1

This is different than the hack in nsHTMLDocument in that it doesn't put the
old root element back in the document once we're done resetting it. Whether or
not we do that might not really matter (IIRC it really did matter in the
nsHTMLDocument hack though), and if it doesn't, that's all nice n' good. But if
we don't need to do that then  I don't see why we can't just call
SetRootContent(nsnull) on the document, that would end up doing the same thing
that this "hack" does (although in different order), i.e. remove all children
from the root content (once the root element is destroyed), you'll still need
to call ContentRemoved(nsnull, root, 0) on the document though.

Either way, as long as this fixes the problem I'm cool with it, it's safe n'
all, so sr=jst, but you might be able to simplify this hack a bit in this case.

Comment 8

[Johnny Stenback (:jst)]

Oh, duh, with attachment 79249 [details] [diff] [review] from bug 56087 this is identical to the
nsHTMLDocument hack.

sr=jst

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

Do what jst said, it works too and is much simpler.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 79592 [details] [diff] [review]
v2

Carrying forward sr=jst, patch was modified according to his first comments.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Address Pike's comment that we should use the loadgroup of the source document
if available.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 79684 [details] [diff] [review]
v2.1

Jst said he was fine with this change

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 79684 [details] [diff] [review]
v2.1

Wrong patch

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 79686 [details] [diff] [review]
v2.1

Jst said he was fine with this change

Comment 16

[Axel Hecht]

Comment on attachment 79686 [details] [diff] [review]
v2.1

one nit, make the 
nsresult result
nsresult rv.
r=[e-mail]

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

When I apply this patch the summary page is completely blank!

Comment 18

[Peter Van der Beken [:peterv]]

I'm seeing the same thing. Turns out I had an additional patch to
nsXMLHTPPRequest that makes this work correctly (and also fixes the "missing
style" problem). I'll attach it now.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Additional fix v1

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 80369 [details] [diff] [review]
Additional fix v1

sr=jst

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 80369 [details] [diff] [review]
Additional fix v1

This patch was written by Heikki (I think, Harish sent it to me). So r=peterv.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Please check the XMLHttpRequest patch with Mitch or someone who knows about
security as well. Specifically, I am not sure if it should be GetOriginalURI()
or GetURI(); what happens if there is a redirect?

After testing I can confirm that I see the scrollbars, so that problem will be
fixed.

However, this introduces a new regression: clicking links in the summary window
that point to anchors in the document is broken. I guess what is happening is
that clicking the link makes us display the original P3P.xml untranformed, and
we scroll in there to the anchor.

Comment 23

[harishd]

Also, inlined SCRIPTs are not executed.

Comment 24

[Peter Van der Beken [:peterv]]

Ok, I know what causes the problem for the links. We change the URL of the
document, but not in the docshell, so the docshell thinks we're not scrolling
but loading a document with a different URL. I'll need to figure out how to fix
the style and scrollbars without regressing the scolling. :-(

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v3

Fixes scrollbars and scripts, scrolling to anchors still works.

Comment 26

[Axel Hecht]

Comment on attachment 80704 [details] [diff] [review]
v3

>Index: mozilla/extensions/transformiix/source/xslt/XSLTProcessor.cpp
<...>
>+#include "nsIScriptGlobalObject.h"
not needed.

>-                        #ifdef PR_LOGGING
>+#ifdef PR_LOGGING
<..>
>-                        #endif
>+#endif

why?

The resulting iframe contains the right stuff, but it's completely black.

I can't even see a scrollbar :-(. Not sure whos bug that is, though.

Comment 27

[Peter Van der Beken [:peterv]]

>>-                        #ifdef PR_LOGGING
>>+#ifdef PR_LOGGING
><..>
>>-                        #endif
>>+#endif
>
>why?

To be consistent with the rest of the code.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Need to always remove the processor as observer from the script loader now.

Comment 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 80830 [details] [diff] [review]
v3.1

r/sr=heikki

Also regarding the move of #foo to the beginning of the line: I seem to recall
that C or C++ spec stated those must start from the beginning of the line.

Comment 30

[harishd]

Comment on attachment 80830 [details] [diff] [review]
v3.1

r=harishd

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: v3.2

The previous code had a race condition :-(. mDocument could be set to null and
then be used when an external script loaded after the transform had finished.
Also, the loadgroup for the result document could be null when called from the
content sink, in that case we should really use the input document's loadgroup.
New patch coming up and apologies to the reviewers.

Comment 32

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin on the 1.0 branch, pending
r/sr= on the new patch. Pls check this in today after getting drivers approval,
then add the fixed1.0.0 keyword.

Comment 33

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 80890 [details] [diff] [review]
v3.2

You need to move #include "nsIScriptLoader.h" to the header file to get this to
compile on Windows and some other platforms.

With that, sr=heikki

Comment 34

[Axel Hecht]

Comment on attachment 80890 [details] [diff] [review]
v3.2

r=[e-mail] with the moved include

patch works, once you have jst's patch from bug 138012

Comment 35

[Jaime Rodriguez, Jr.]

Based on Axel Comment #34, should this bug be dependent on bug 138012, in
addition to bug 56087?

Comment 36

[Peter Van der Beken [:peterv]]

Just to clarify, the fix for bug 138012 fixes the testcase attached in this bug.
The P3P policy viewer is not affected by bug 138012. I'm not marking a
dependency since this bug is originally about the P3P policy viewer.

Comment 37

[Peter Van der Beken [:peterv]]

Sent mail to drivers for checkin to the branch.

Comment 38

[Peter Van der Beken [:peterv]]

Checked in on the trunk.

Comment 39

[Jimmy Lee]

Build: 2002-04-26-08-trunk(WIN), 2002-04-26-08-trunk(MAC 9.x/10.x),
2002-04-26-10-trunk(LINUX)

We have scroll bars in Summary window.  Marking Verified for TRUNK.

Comment 40

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 80890 [details] [diff] [review]
v3.2

a=dbaron for 1.0 branch checkin