Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Accessing cross-domain document structure via TreeWalker

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
DOM: Core & HTML
--
major
VERIFIED FIXED
15 years ago
4 years ago

People

(Reporter: bbaetz, Assigned: sicking)

Tracking

Trunk
mozilla1.0.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT1 RTM] [ETA 08/02] security)

Attachments

(5 attachments, 21 obsolete attachments)

961 bytes, text/html
Details
654 bytes, text/html
Details
581 bytes, text/html
Details
763 bytes, text/html
Details
62.34 KB, patch
sicking
: review+
sicking
: superreview+
jesup
: approval+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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?
(Reporter)

Comment 1

15 years ago
Created attachment 90607 [details]
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?
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.
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|?
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://...").
Created attachment 90934 [details] [diff] [review]
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 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.
Attachment #90934 - Flags: review+
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.
Attachment #90934 - Flags: superreview+

Comment 8

15 years ago
What other DOM stuff uses this akward syntax?  I know that at least one other
function, document.getAnonymousNodes, uses it.
Created attachment 91079 [details] [diff] [review]
fixes jsts comment
Attachment #90934 - Attachment is obsolete: true
Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

carrying over r/sr
Attachment #91079 - Flags: superreview+
Attachment #91079 - Flags: review+
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...
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

15 years ago
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.
taking...
Assignee: anthonyd → sicking
adding keywords, important security bug. nsbeta1+'ed with heikkis blessing
Keywords: adt1.0.1, mozilla1.0.1, nsbeta1+
Whiteboard: [ADT1 RTM]

Comment 16

15 years ago
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.
Attachment #91079 - Flags: approval+

Updated

15 years ago
Whiteboard: [ADT1 RTM] → [ADT1 RTM] [eta=7-17]
Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

checked in on trunk

Comment 18

15 years ago
Reducing to adt2 per adt.
Whiteboard: [ADT1 RTM] [eta=7-17] → [ADT2 RTM] [eta=7-17]

Comment 19

15 years ago
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.

Updated

15 years ago
Blocks: 143047
Whiteboard: [ADT2 RTM] [eta=7-17] → [ADT2 RTM] [eta 07/17]
Created attachment 91442 [details] [diff] [review]
Plug more holes...
Created attachment 91479 [details] [diff] [review]
...and yet more (requires attachment 91442 [details] [diff] [review])
Created attachment 91485 [details] [diff] [review]
combined and refined
Attachment #91442 - Attachment is obsolete: true
Attachment #91479 - Attachment is obsolete: true
Comment on attachment 91485 [details] [diff] [review]
combined and refined

r=jst for the parts I didn't write myself.
Attachment #91485 - Flags: review+

Comment 24

15 years ago
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.
Created attachment 91491 [details] [diff] [review]
fixes bzs comments
Attachment #91485 - Attachment is obsolete: true

Comment 26

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

sr=bzbarsky
Attachment #91491 - Flags: superreview+
also changed the transformiix stuff to
    if (!URIUtils::CanCallerAccess(aSourceDOM) ||
        !URIUtils::CanCallerAccess(aStyleDOM) ||
        !URIUtils::CanCallerAccess(aOutputDoc))
        return NS_ERROR_DOM_SECURITY_ERR;
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?
Created attachment 91507 [details] [diff] [review]
Patch to disable the security checks temporarily
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.
I checked in attachment 91507 [details] [diff] [review].
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...
Created attachment 91527 [details] [diff] [review]
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.
Created attachment 91528 [details] [diff] [review]
diff -w of the above
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.
+    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
other then that r=sicking
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 on attachment 91527 [details] [diff] [review]
Fix the performance issues

ok, r=sicking
Attachment #91527 - Flags: review+

Comment 40

15 years ago
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).
Attachment #91527 - Flags: superreview+
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.
Created attachment 91597 [details] [diff] [review]
XSLT/XPath changes
Comment on attachment 91597 [details] [diff] [review]
XSLT/XPath changes

sr=jst
Attachment #91597 - Flags: superreview+
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.
Attachment #91597 - Flags: review+

Updated

15 years ago
Whiteboard: [ADT2 RTM] [eta 07/17] → [ADT2 RTM] [eta 07/18]
Created attachment 91748 [details] [diff] [review]
combines 91527 and 91597

combines 91527 and 91597 and makes a small change to ::CheckSameOrigin as
discussed on irc
Attachment #91527 - Attachment is obsolete: true
Attachment #91528 - Attachment is obsolete: true
Attachment #91597 - Attachment is obsolete: true
arg, forgot to do petervs change, The new code is

rv = CallGetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &sSecurityManager);
Created attachment 91753 [details] [diff] [review]
*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
Attachment #91748 - Attachment is obsolete: true

Comment 48

15 years ago
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
Attachment #91753 - Flags: superreview+
Comment on attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

r=jst
Attachment #91753 - Flags: review+
Created attachment 91757 [details] [diff] [review]
fixes bzs comments

this also fixes nsXULElement::GetOwnerDocument which is needed to not regress
some xul stuff, got r=jst over irc for that
Attachment #91753 - Attachment is obsolete: true
Comment on attachment 91757 [details] [diff] [review]
fixes bzs comments

bringing forward r=jst
Attachment #91757 - Flags: review+

Comment 52

15 years ago
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
Attachment #91757 - Flags: superreview+

Comment 53

15 years ago
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.
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.
Status: NEW → ASSIGNED
the checkin for this caused bug 158202, should be fixed soon
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
QA Contact: lchiang → bsharma
Created attachment 91903 [details]
testcase for some of the exploits fixed
Created attachment 91904 [details]
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.

Updated

15 years ago
Blocks: 158340
crash bug 158340 is being blamed on this
Raising priority to ADT1 based on jst's comments in email
Whiteboard: [ADT2 RTM] [eta 07/18] → [ADT1 RTM] [eta 07/18]

Comment 61

15 years ago
this is blamed for the outstanding smoketest blocker: bug 158167 (not a crash)
Blocks: 158167, 158202
Severity: normal → major
OS: Linux → All
Hardware: PC → All
Whiteboard: [ADT1 RTM] [eta 07/18] → [ADT2 RTM] [eta 07/18]

Updated

15 years ago
Whiteboard: [ADT2 RTM] [eta 07/18] → [ADT1 RTM] [eta 07/18]

Comment 62

15 years ago
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? 
Keywords: adt1.0.1
Whiteboard: [ADT1 RTM] [eta 07/18] → [ADT1 RTM] [eta 07/23]
Target Milestone: --- → mozilla1.0.1
Created attachment 92260 [details] [diff] [review]
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...
Created attachment 92292 [details] [diff] [review]
fix XUL problem version 2

addressed comments from jst
Attachment #92260 - Attachment is obsolete: true
Comment on attachment 92292 [details] [diff] [review]
fix XUL problem version 2

sr=jst
Attachment #92292 - Flags: superreview+

Comment 66

15 years ago
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.
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).
Created attachment 92316 [details] [diff] [review]
Fix bzs comments
Attachment #92292 - Attachment is obsolete: true
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

bring forward jsts sr
Attachment #92316 - Flags: superreview+

Comment 70

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

r=bzbarsky and all that.
Attachment #92316 - Flags: review+
Created attachment 92372 [details] [diff] [review]
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
Attachment #91079 - Attachment is obsolete: true
Attachment #91491 - Attachment is obsolete: true
Attachment #91507 - Attachment is obsolete: true
Attachment #91757 - Attachment is obsolete: true
Blocks: 158377

Comment 72

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

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92316 - Flags: approval+

Comment 73

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

r=bzbarsky
Attachment #92372 - Flags: review+
Another regression found: bug 158766. Jst is working on it.

Comment 75

15 years ago
Created attachment 92482 [details]
demo that reads text from a form field using appendChild and dom 0
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.

Updated

15 years ago
Whiteboard: [ADT1 RTM] [eta 07/23] → [ADT1 RTM] [ETA 07/25]
Created attachment 92515 [details] [diff] [review]
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 on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

r=sicking
Attachment #92515 - Flags: review+

Comment 79

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

sr=bzbarsky
Attachment #92515 - Flags: superreview+
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

checked in earlier today
Attachment #92316 - Attachment is obsolete: true

Comment 81

15 years ago
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
Attachment #92372 - Flags: approval+

Comment 82

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

a=chofmann for branch and trunk.
Attachment #92515 - Flags: approval+

Updated

15 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+
DOM inspector regression fixed on the trunk.
Created attachment 92542 [details] [diff] [review]
Complete diff for the branch.

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

Updated

15 years ago
Keywords: adt1.0.1

Comment 85

15 years ago
Bindu, can you verify this on the trunk?

Comment 86

15 years ago
changing the branch landing eta to 7/29 since we'll be letting this bake on the
trunk for a few more days.
Whiteboard: [ADT1 RTM] [ETA 07/25] → [ADT1 RTM] [ETA 07/29]

Comment 87

15 years ago
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!
Keywords: adt1.0.1

Updated

15 years ago
Depends on: 159348
The new regression is bug 159348. A patch exists (by sicking and mstoltz), but
it's not attached to a bug yet, coming soon.
... 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)... :-(

Updated

15 years ago
Depends on: 159468
Created attachment 92973 [details] [diff] [review]
Patch for the branch, including the fix for bug 159468
Attachment #92542 - Attachment is obsolete: true
Created attachment 92977 [details]
stacktrace of new principal code

Updated

15 years ago
Whiteboard: [ADT1 RTM] [ETA 07/29] → [ADT1 RTM] [ETA 08/01]

Comment 92

15 years ago
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!
Keywords: adt1.0.1, approval

Comment 93

15 years ago
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

15 years ago
Adding Siva to the list. He has some DOM tests that he can run.
Attachment #92515 - Attachment is obsolete: true
marking this one fixed on request from adt and that it actually is fixed on the
trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Created attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348
Attachment #92372 - Attachment is obsolete: true
Attachment #92973 - Attachment is obsolete: true
Attachment #92977 - Attachment is obsolete: true

Comment 97

15 years ago
can the new patch go as is, or will we need to get new reviews?
Comment on attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348

This has got r=bz and sr=jst
Attachment #93512 - Flags: superreview+
Attachment #93512 - Flags: review+

Comment 99

15 years ago
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!
Keywords: adt1.0.1 → adt1.0.1+
I'm confused. Who is supposed to change mozilla1.0.1+ to fixed1.0.1? Me or
Siva/Bindu?
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 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.
Attachment #93512 - Flags: approval+

Updated

15 years ago
Whiteboard: [ADT1 RTM] [ETA 08/01] → [ADT1 RTM] [ETA 08/02]

Comment 103

15 years ago
verfied on trunk.
Status: RESOLVED → VERIFIED

Comment 104

15 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
bsharma: what branch-build did you test this with? This was checked in on the
branch just an hour ago.

Comment 106

15 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 107

15 years ago
marking verified, needs to verify on branch after mozilla1.0.1 is removed and
fixed1.0.1 is added.
Status: RESOLVED → VERIFIED
checked in and cycled green
Keywords: mozilla1.0.1+ → fixed1.0.1

Comment 109

15 years ago
verified on branch 08-02-06-1.0. adding keyword verified1.0.1
Keywords: fixed1.0.1 → verified1.0.1
Group: security?
Group: security?
Group: security

Updated

14 years ago
Whiteboard: [ADT1 RTM] [ETA 08/02] → [ADT1 RTM] [ETA 08/02] security
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.