156452 - Accessing cross-domain document structure via TreeWalker

Status: VERIFIED FIXED

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

Description

[Bradley Baetz (:bbaetz)]

Its possible to use the tree walker to get at the document structure of a
document in another domain. I'll attach a testcase (based on sicking's original
tests).

I don't think its possible to see data/attributes from the nodes without finding
a separate bug somehow - I get permission denied errors whenever I try to access
anything on the nodes themselves, apart from .toString. I haven't tried anything
fancy, though.

Is it legal to call document.createTreeWalker(someOtherDocument, ...) in the
first place?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: testcase

This is basically sicking's original testcase, modified. To use, wait until the
contents of the <iframe> have loaded, then click |begin|. Then traverse the DOM
tree using the other buttons.

Can anyone manage to get useful information out of this?

Comment 2

[Johnny Stenback (:jst)]

Hmm, I can't see how this would be directly exploitable, but we should block it
in any case. This could be fixed the same way bug 147754 was fixed.

Comment 3

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

How can the user get hold of the node to initialize the treewalker to in the
first place? I.e. shouldn't it fail already at the
|document.getElementById('f').contentDocument|?

Comment 4

[Johnny Stenback (:jst)]

No, accessing .contentDocument should be allowed since there are properties on
the document in the iframe that anyone should be able to set (like
.contentDocument.location = "http://...").

Comment 5

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

Attachment: patch to fix

This patch performs same-origin tests whenever we create a treewalker of set
the contentnode in a treewalker. Those two should be the only methods of
setting the node that the walker walks

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 90934 [details] [diff] [review]
patch to fix

Perfect. The |if (doc1 == doc2)| fast return path is a good thing, and your use
of nsScriptSecurityManager looks fine. r=mstoltz.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 90934 [details] [diff] [review]
patch to fix

...
+  nsresult rv = nsContentUtils::CheckSameOrigin(this, aRoot);
+  NS_ENSURE_SUCCESS(rv, rv);

Don't use NS_ENSURE_SUCCESS() in places where you know the call can fail for
valid reasons, like in this case. if (NS_FAILED(rv)) { return NS_OK; } in
stead, but on separate lines...

sr=jst if you change that.

Comment 8

[Jesse Ruderman]

What other DOM stuff uses this akward syntax?  I know that at least one other
function, document.getAnonymousNodes, uses it.

Comment 9

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

Attachment: fixes jsts comment

Comment 10

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

Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

carrying over r/sr

Comment 11

[Johnny Stenback (:jst)]

Hmm, what if script from domain X gets access to a document from domain Y and is
able to reach an element in the document from domain Y, could it then do
document.body.appendChild() to steal the node from the other document?

There's also document.importNode(), and maybe others...

Comment 12

[Johnny Stenback (:jst)]

We might want new bugs for those issues if they're real issues... Seems like all
properties of document objects that expose elements are protected, but who knows
if there are other ways to reach an element in a window to which you're not
reall supposed to have access to... Belts and suspenders, we need some of that
here it seems...

Comment 13

[Jesse Ruderman]

Sorry for the incoherent comment earlier.  What I meant was, "what other DOM
functions are accessed using document.functionName but don't necessarily operate
on the document itself?"  A better question is, "what other DOM functions can
take a document as a parameter and might need a security check on that
parameter?".  

After looking at the traversal spec and the testcase and scratching my head for
a few minutes: Eek! I didn't know Document inherited from Node.  That blows away
my assumption that a script can get access to another document's document object
but not its nodes.  That means we have a lot of functions to consider and/or add
security checks to.

Comment 14

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

taking...

Comment 15

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

adding keywords, important security bug. nsbeta1+'ed with heikkis blessing

Comment 16

[chris hofmann]

Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

a=chofmann for the 1.1b trunk...  lets see how if goes there and then look at
getting on the branch.

Comment 17

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

Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

checked in on trunk

Comment 18

[Paul Wyskoczka]

Reducing to adt2 per adt.

Comment 19

[lchiang]

gerardok or bsharma - can you take over as the QA contact and verify the bug fix
with the attached testcase on the trunk builds (after July 15)?   Also, will
need verification once the fix is in the branch.  Thanks.

Comment 20

[Johnny Stenback (:jst)]

Attachment: Plug more holes...

Comment 21

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

Attachment: ...and yet more (requires attachment 91442)

Comment 22

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

Attachment: combined and refined

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 91485 [details] [diff] [review]
combined and refined

r=jst for the parts I didn't write myself.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91485 [details] [diff] [review]
combined and refined

>     aNode1->GetOwnerDocument(getter_AddRefs(domDoc1));
>     doc1 = do_QueryInterface(domDoc1);
>     if (!doc1) {
>-      return NS_ERROR_FAILURE;
>+      // aNode1 is not part of a document, let any caller access it.
>+      return NS_OK;
>     }

I would be happier with:

  aNode1->GetOwnerDocument(getter_AddRefs(domDoc1));
  if (!domDoc1) {
    // aNode1 is not part of a document, let any caller access it.
    return NS_OK;
  }
  doc1 = do_QueryInteface(domDoc1);
  NS_ASSERTION(doc1, "We have an nsIDOMDocument that's not QIable to
nsIDocument");

or something along those lines.  That way a QI failure won't suddenly open up
access to stuff.

Same thing for doc2, of course, and same in nsContentUtils::CanCallerAccess

> +    if(NS_FAILED(rv)) {

Space after the "if", please?  You have this in a few places.

> nsDocument::AddBinding(nsIDOMElement* aContent, const nsAString& aURL)

How about fixing nsDocument::RemoveBinding too?

>+  if (old_doc != mDocument) {
>+    if(!nsContentUtils::CanCallerAccess(aNewChild)) {

This strikes me as a good place to use boolean operators instead of nested
ifs... ;)

>+  if (old_doc != mDocument) {
>+    if(!nsContentUtils::CanCallerAccess(aNewChild)) {

Here too.


>+MBool URIUtils::CanCallerAccess(nsIDOMNode *aNode)

Same comments apply here as to nsContentUtils::CanCallerAccess

>+#else

#else /* TX_EXE */

> #endif

#endif /* TX_EXE */

Makes things more readable, imo.

Comment 25

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

Attachment: fixes bzs comments

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91491 [details] [diff] [review]
fixes bzs comments

sr=bzbarsky

Comment 27

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

also changed the transformiix stuff to
    if (!URIUtils::CanCallerAccess(aSourceDOM) ||
        !URIUtils::CanCallerAccess(aStyleDOM) ||
        !URIUtils::CanCallerAccess(aOutputDoc))
        return NS_ERROR_DOM_SECURITY_ERR;

Comment 28

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

This patch increased new window time on tinderbox by around 20%-25% on the
mid-speed tinderboxes and by 47% on the fastest tinderbox (comet).  (The slow
ones haven't cycled yet.)  That seems a bit excessive.  Any thoughts on how to
fix it?

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: Patch to disable the security checks temporarily

Comment 30

[Peter Van der Beken [:peterv]]

Also, please correct the indentation in txURIUtils (4 spaces instead of 2) and
share CanCallerAccess (for example exposing it through the security manager)
instead of implementing it twice.

Comment 31

[Peter Van der Beken [:peterv]]

I checked in attachment 91507 [details] [diff] [review].

Comment 32

[Johnny Stenback (:jst)]

Ugh, that sucks... I'll have a look and see why we end up hitting this new
security check so often, updating my tree now...

Comment 33

[Johnny Stenback (:jst)]

Attachment: Fix the performance issues

Looks like the performance hit was due to the security checks in
nsXULCommandDispatcher::AddCommandDispatcher() and
nsXULDocument::AddBroadcastListenerFor(), so I changed those to use the faster
version of the security check which is more or less a nop in the common case
(every case during startup), and I also optimized the checks in
nsGenericElement a bit more. Also, I added a cache for the security manager
pointer to nsContentUtils to avoid hitting the component manager for every
security check.

Comment 34

[Johnny Stenback (:jst)]

Attachment: diff -w of the above

Comment 35

[Johnny Stenback (:jst)]

CanCallerAccess(nsIDOMNode *) does IMO *not* belong in caps. Had it been more
than a trivial ammount of code we could share it, but given how little code it
is I don't think it's worth it.

Comment 36

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

+    if (!doc2) {
+      // aNode is neither a nsIContent nor an nsIDocument, something
+      // weird is going on...
+
+      NS_ERROR("aNode is neither an nsIContent nor an nsIDocument!");
+
+      return NS_ERROR_UNEXPECTED;

Why not do the same as for aNode1 and say that this is a faked node? I.e. return
NS_ERROR_DOM_SECURITY_ERR. Same in nsContentUtils::CanCallerAccess


Shouldn't you release the security-manager somewhere? though i can't find any
place where sXPConnect is release either...


I'll try to come up with some code to speed up the XSLT stuff

Comment 37

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

other then that r=sicking

Comment 38

[Johnny Stenback (:jst)]

I decided to return NS_ERROR_UNEXPECTED in stead of NS_ERROR_DOM_SECURITY_ERR
since we don't know for a fact that the case where that code would be triggerd
is a security error, it might be, but it could be for some other reason as well.

And yes, the cached security manager needs to be released in
nsContentUtils::Shutdown() (where sXPConnect is released too btw). Consider that
change made...

Comment 39

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

Comment on attachment 91527 [details] [diff] [review]
Fix the performance issues

ok, r=sicking

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91527 [details] [diff] [review]
Fix the performance issues

It may be a good idea to move the |if (!sSecurityManager)| checks up to before
we do all that document-getting.  Doesn't affect us, but may affect some
embedding apps.

other than that, sr=bzbarsky (assuming that change to Shutdown(), of course).

Comment 41

[Johnny Stenback (:jst)]

No, I intentionally put the if (!sSecurityManager) check far down in the method
so that we don't bother doing that check in the fast path (i.e. get the document
pointers and compare, and return early in the common case). It only matters for
embedders who choose to not have a security manager, which would be a really bad
decision IMO.

Comment 42

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

Attachment: XSLT/XPath changes

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 91597 [details] [diff] [review]
XSLT/XPath changes

sr=jst

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 91597 [details] [diff] [review]
XSLT/XPath changes

>Index: extensions/transformiix/build/XSLTProcessorModule.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/build/XSLTProcessorModule.cpp,v
>retrieving revision 1.21
>diff -u -r1.21 XSLTProcessorModule.cpp
>--- extensions/transformiix/build/XSLTProcessorModule.cpp	15 May 2002 18:53:45 -0000	1.21
>+++ extensions/transformiix/build/XSLTProcessorModule.cpp	17 Jul 2002 04:27:03 -0000

>@@ -192,7 +195,17 @@
>         return NS_ERROR_OUT_OF_MEMORY;
>     if (!txHTMLAtoms::init())
>         return NS_ERROR_OUT_OF_MEMORY;
>+
>+    rv = nsServiceManager::GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID,
>+                                      nsIScriptSecurityManager::GetIID(),
>+                                      (nsISupports **)&sSecurityManager);
>+    if (NS_FAILED(rv)) {
>+        sSecurityManager = nsnull;
>+        return rv;
>+    }
>+

Please use CallGetService.

r=peterv.

Comment 45

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

Attachment: combines 91527 and 91597

combines 91527 and 91597 and makes a small change to ::CheckSameOrigin as
discussed on irc

Comment 46

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

arg, forgot to do petervs change, The new code is

rv = CallGetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &sSecurityManager);

Comment 47

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

Attachment: *this* is really what i ment to attach

apparently i had forgot to save in a few more places, this should be the real
deal

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

>+  /*
>+   * Checks if two nodes comes from the same origin. aTrustedNode is

Make that two '*' chars on the first line so doxygen picks it up?

"whether", not "if".

>+   * considered 'safe' in that a user can operate on it and that it isn't
>+   * isn't a js-object that implements nsIDOMNode.

Take out one "isn't".

>+ * Never call this function with the first node provided by script, it
>+ * must always be known to be a 'real' node!

Could we add a debug-only assertion that aTrustedNode is either an nsIDocument
or nsIContent?	Just to catch people who don't read comments?

More importantly, please add NS_PRECONDITION(aTrustedNode, "There must be a
trusted node"), since you dereference it unconditionally when it's not an
nsIDocument.

You never release sScriptSecurityManager in nsContentUtils::Shutdown()

>Index: content/base/src/nsDOMAttributeMap.cpp

Replace the code you removed from here with "// XXX" comments to readd it once
GetOwnerDocument gets fixed on Attr nodes.  File a bug to that effect, make it
depend on the relevant Attr node bug.

>+extern nsIScriptSecurityManager *sSecurityManager;

This may make more sense as gSecurityManager, since it's not static....  But
whatever the module convention is. ;)

sr=bzbarsky with those changes

Comment 49

[Johnny Stenback (:jst)]

Comment on attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

r=jst

Comment 50

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

Attachment: fixes bzs comments

this also fixes nsXULElement::GetOwnerDocument which is needed to not regress
some xul stuff, got r=jst over irc for that

Comment 51

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

Comment on attachment 91757 [details] [diff] [review]
fixes bzs comments

bringing forward r=jst

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91757 [details] [diff] [review]
fixes bzs comments

sr=bzbarsky if	you fix the nsContentUtils.h comment to fix that in
nsContentUtils.cpp

Comment 53

[Kathleen :Brade]

Apologies if I'm confused about the destination of this patch; however, if this
latest patch is intended for the branch, it is incomplete.  It needs the
Composer fix in editor.js (bug 157851; patch 91702) unless that issue has been
addressed in some other way.

Comment 54

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

brade: The patch from that bug should not be needed any more. However it's still
a good thing that it landed on trunk since the new code is faster, cleaner and
smaller.

Comment 55

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

the checkin for this caused bug 158202, should be fixed soon

Comment 56

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

XUL prototypes are also causing us trouble, we havn't yet ran into a regression
from this, but there is a possibility for it. We're looking into how to fix this

Comment 57

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

Attachment: testcase for some of the exploits fixed

Comment 58

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

Attachment: another testcase

clicking "dump string contents" will dump the string contents of the url in the
iframe. I even whitespace-normalised it so that it would be easier to read :)

With the patch we throw a security exception.

In attachment 91903 [details] pressing "clone contents" will clone all contents of the
iframe into the main document. Again, the patch makes us throw a security
exception instead.

Comment 59

[Daniel Veditz [:dveditz]]

crash bug 158340 is being blamed on this

Comment 60

[Mitchell Stoltz (not reading bugmail)]

Raising priority to ADT1 based on jst's comments in email

Comment 61

[timeless]

this is blamed for the outstanding smoketest blocker: bug 158167 (not a crash)

Comment 62

[Jaime Rodriguez, Jr.]

Removing adt1.0.1, as this has been said to cause smoketest blocker bug 158167.  

It seems that this has caused a couple of regressions each time it has landed on
the trunk. what steps can we take to make sure taking this fix this late date
does run the risk of breaking something else? 

Comment 63

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

Attachment: fix XUL problem

This fixes a know XUL problem. I'm not sure if this problem actually has
regressed anything in mozilla (I havn't heard of anyone reporting a problem
that could be traced back to this) but better stay on the safe side...

Comment 64

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

Attachment: fix XUL problem version 2

addressed comments from jst

Comment 65

[Johnny Stenback (:jst)]

Comment on attachment 92292 [details] [diff] [review]
fix XUL problem version 2

sr=jst

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92292 [details] [diff] [review]
fix XUL problem version 2

>+  /*

Two stars on this first line, please (doxygen syntax).

>+   * Sets the url of the nodeinfo manager. This should only be called when
>+   * this nodeinfomanager isn't connected to an nsIDocument.

Pick a spelling of "nodeinfo manager" and stick with it?  ;)

>+   * Get hold of each nodes document or uri
>+   */

"node's", not "nodes"

>+      // In theory this should never happen. But since theory and reality is
>+      // different for XUL elements we'll try to get the URI from the

"are different"

>+      nsCOMPtr<nsINodeInfoManager> nimgr;
>+      ni->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+      nimgr->GetDocumentURL(getter_AddRefs(trustedUri));

Can getting nimgr fail or give null here?

>+      nsCOMPtr<nsINodeInfoManager> nimgr;
>+      ni->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+      nimgr->GetDocumentURL(getter_AddRefs(unTrustedUri));

Same here.

>+  if (!trustedUri) {
>+    trustedDoc->GetDocumentURL(getter_AddRefs(trustedUri));

Why is trustedDoc not null here?  What if the nodeinfo manager just gave us a
null URI?

>+  if (!unTrustedUri) {
>+    unTrustedDoc->GetDocumentURL(getter_AddRefs(unTrustedUri));

Same here.

>@@ -552,7 +552,6 @@
>     }
>   }
> 
>-  NS_IF_RELEASE(mDocumentURL);
>   NS_IF_RELEASE(mPrincipal);
>   mDocumentLoadGroup = nsnull;
> 
>@@ -631,6 +630,8 @@
>   if (mNodeInfoManager) {
>     mNodeInfoManager->DropDocumentReference();
>   }
>+
>+  NS_IF_RELEASE(mDocumentURL);
> }

Add a comment explaining why this change was needed.

> nsNodeInfoManager::DropDocumentReference()
> {
>+  if (mDocument) {

Is this ever actually called when mDocument is null?  (Not that I mind the
null-check, but if an assertion would do then we should do that instead.)

>+NS_IMETHODIMP
>+nsNodeInfoManager::GetDocumentURL(nsIURI** aURL)
>+{
>+  NS_ENSURE_ARG_POINTER(aURL);

Please assert that !mDocument || !mDocumentURL.  It would be even better if we
could assert that (mDocument || mDocumentURL) && (!mDocument ||
!mDocumentURL).... (but I doubt we can do that).

Please clear mDocumentURL in Init() as we discussed.

Comment 67

[Johnny Stenback (:jst)]

An nsINodeInfo will never give back a null node info manager, and it will never
fail to give you a node info manager (unless someone uses a node info that was
never initialized, but we don't need to protect against code that broken).

Comment 68

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

Attachment: Fix bzs comments

Comment 69

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

Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

bring forward jsts sr

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

r=bzbarsky and all that.

Comment 71

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

Attachment: patch for branch

This is a patch for branch which is a and is a merge of all patches that has
landed in relation to this. Including the patches in bug 158202 and bug 158167

Comment 72

[Asa Dotzler [:asa]]

Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

a=asa (on behalf of drivers) for checkin to 1.1

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92372 [details] [diff] [review]
patch for branch

r=bzbarsky

Comment 74

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

Another regression found: bug 158766. Jst is working on it.

Comment 75

[Jesse Ruderman]

Attachment: demo that reads text from a form field using appendChild and dom 0

Comment 76

[Mitchell Stoltz (not reading bugmail)]

The testcase Jesse just posted in comment 75 reads the contents of a form on a
page from another host using only the core DOM function appendChild. This is
very serious and directly exploitable, so it's a stop-ship.

Comment 77

[Johnny Stenback (:jst)]

Attachment: Fix the DOM inspector regression

This fixes the last known regression caused by this whole security fix.

This change makes sure that the correct JS context is pushed onto the JS stack
when we're focusing and blurring elements from native code. This change does
that by creating a new little JS context "pusher" class that uses existing and
well tested code to do the pushing and popping of the JS context when event
handlers are called. So the bulk of this change is to move exisiting code into
a new class and make the code that used the moved code use the new class, and
then also use the new class in the code that deals with focus and blur.

Comment 78

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

Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

r=sicking

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

sr=bzbarsky

Comment 80

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

Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

checked in earlier today

Comment 81

[chris hofmann]

Comment on attachment 92372 [details] [diff] [review]
patch for branch

a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking into the branch

Comment 82

[chris hofmann]

Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

a=chofmann for branch and trunk.

Comment 83

[Johnny Stenback (:jst)]

DOM inspector regression fixed on the trunk.

Comment 84

[Johnny Stenback (:jst)]

Attachment: Complete diff for the branch.

This is a diff against the branch, this includes all the above changes,
including all regression fixes.

Comment 85

[scottputterman]

Bindu, can you verify this on the trunk?

Comment 86

[scottputterman]

changing the branch landing eta to 7/29 since we'll be letting this bake on the
trunk for a few more days.

Comment 87

[Jaime Rodriguez, Jr.]

jst mentioned that there was another regression related to this checkin.
therefore, i am removing adt1.0.1 nomination until the new regression is
addressed. pls renominate when you think the new patch is ready for the branch.
thanks!

Comment 88

[Johnny Stenback (:jst)]

The new regression is bug 159348. A patch exists (by sicking and mstoltz), but
it's not attached to a bug yet, coming soon.

Comment 89

[Johnny Stenback (:jst)]

... and the saga continues, one of my fixes for one of the regressions here
caused yet one more regression. Bug 159468 is the newest bug in this sad story
(fix attached)... :-(

Comment 90

[Johnny Stenback (:jst)]

Attachment: Patch for the branch, including the fix for bug 159468

Comment 91

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

Attachment: stacktrace of new principal code

Comment 92

[Jaime Rodriguez, Jr.]

bsharma: Bindu, looks like the patch for the remaining regression has been
checked into the trunk. Can you pls run DOM tests, and check around for any
possible regressions that might be caused by this fix. If there are none, pls
verify this bug as fixed. thanks!

sicking: Jonas, now that the last regression has been patched, can you resolve
this bug as fixed, and run a diff for 1.0 branch through the review and approval
gauntlet, so that we are ready to check this one in tonight? thanks!

Comment 93

[bsharma]

Verified on 2002-07-31-trunk build in Win 2000.

Ran following tests and they all give an exception.

1. Test case in comment #1
2. Test case in comment #57 and #58
3. Test case in comment #75.
4. Tests in bug 90757

Comment 94

[bsharma]

Adding Siva to the list. He has some DOM tests that he can run.

Comment 95

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

marking this one fixed on request from adt and that it actually is fixed on the
trunk

Comment 96

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

Attachment: patch for branch, including fix for bug 159348

Comment 97

[Jaime Rodriguez, Jr.]

can the new patch go as is, or will we need to get new reviews?

Comment 98

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

Comment on attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348

This has got r=bz and sr=jst

Comment 99

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) apporval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then replace "mozilla1.0.1+" with
"fixed1.0.1". thanks!

Siva and Bindu: Let's finish the test on the trunk tomorrow, and then focus our
efforts on the branch. Once you have run all your tests on the branch, feel this
is fixed, and that no new regressions were caused by this fix, then pls replace
"mozilla1.0.1+" with "fixed1.0.1". thanks!

Comment 100

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

I'm confused. Who is supposed to change mozilla1.0.1+ to fixed1.0.1? Me or
Siva/Bindu?

Comment 101

[Jaime Rodriguez, Jr.]

doh! sorry my bad. apologies all around. 

jonas, you are *of course* suppose to replace "mozilla1.0.1+" with "fixed1.0.1".

once qa verifies the fix on the 1.0 branch, they will replace "fixed1.0.1", with
"verified1.0.1".

again, my apologies.

Comment 102

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348

re-approval for branch; change mozilla1.0.1+ to fixed1.0.1 when checked in. 
Please watch Txul, Ts, and Tp after this is checked in for regressions on
branch.

Comment 103

[Sivakiran Tummala]

verfied on trunk.

Comment 104

[bsharma]

Verified on 2002-08-01-branch on Win 2000.

1. Test case in Comment #1: Dialog bix appears with the Object names when
clicked on several buttons.
2. Test case in Comment #57: The another Google is opened in the lower frame
when clicked on the Clone contents button. But what I typed in the search box is
not displayed.
3. Test case in Comment #58: When clicked on "dump string content" button, the
contents of Google window are opened in the lower frame.
4. Test case in Comment #75: Typed search string in the Google window, when
clicked on the button, an alert is appeared with the test string from Google
window in it.
4. Tests in bug 90757, all the testse are passed.

Comment 105

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

bsharma: what branch-build did you test this with? This was checked in on the
branch just an hour ago.

Comment 106

[Sivakiran Tummala]

bindu, 
it is to be verified on branch only after fixed1.0.1 is added to the keyword
list. I think they are still waiting for the drivers approval to move the patch
on to branch. I am marking this bug Resolved/verified fixed again.

Comment 107

[Sivakiran Tummala]

marking verified, needs to verify on branch after mozilla1.0.1 is removed and
fixed1.0.1 is added.

Comment 108

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

checked in and cycled green

Comment 109

[Sivakiran Tummala]

verified on branch 08-02-06-1.0. adding keyword verified1.0.1