Last Comment Bug 333697 - Gmail (https-only) won't properly load
: Gmail (https-only) won't properly load
Status: VERIFIED FIXED
regression from 321299 (fix incorpora...
: fixed1.8.0.7, fixed1.8.1, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 major with 4 votes (vote)
: mozilla1.8.1beta2
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
https://mail.google.com/mail/
: 334475 334840 (view as bug list)
Depends on: 335785 356851
Blocks: 321299
  Show dependency treegraph
 
Reported: 2006-04-12 03:52 PDT by Peter van der Woude [:Peter6]
Modified: 2007-04-05 13:38 PDT (History)
25 users (show)
mbeltzner: blocking1.8.1+
dveditz: blocking1.8.0.4-
dveditz: blocking1.8.0.5-
dveditz: blocking1.8.0.7+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial fix (18.68 KB, patch)
2006-04-13 16:33 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
jst: superreview+
dveditz: approval‑branch‑1.8.1-
dveditz: approval1.8.0.4-
dveditz: approval1.8.0.5-
Details | Diff | Review

Description Peter van der Woude [:Peter6] 2006-04-12 03:52:48 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060411 Firefox/3.0a1 ID:2006041111 [both Cairo and non-Cairo]

repro:
1.Open FF
2.Open Gmail

result:
-sometimes you're stuck on a blank page
-sometimes the main window loads but all links are dead
-sometimes it works

also happens in -safe-mode

reproducable: not all the time

regressionwindow:
works in 20060410 nightly
fails in 20060411 nightly
Comment 1 Peter van der Woude [:Peter6] 2006-04-12 03:55:40 PDT
actually, the links do work... the screen simply refuses to refresh.
nothing in the errorconsole
Comment 2 Uri Bernstein (Google) 2006-04-12 05:44:54 PDT
I saw this yesterday with the 2006-04-11 Mac (PPC) nightly (but not with a 2006041012 build).
Comment 3 Patrick 2006-04-12 08:01:15 PDT
Are you using http or https?

Comment 4 Peter van der Woude [:Peter6] 2006-04-12 09:14:39 PDT
(In reply to comment #3)
> Are you using http or https?
> 
allways https
Comment 5 Peter van der Woude [:Peter6] 2006-04-12 09:16:09 PDT
in http it seems fine (but i dont consider that an acceptable workaround)
Comment 6 Patrick 2006-04-12 09:17:16 PDT
Agreed, but it helps to describe the actual problem better :)
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-12 09:53:00 PDT
No idea.  Ask in that bug?  I wonder why (or whether) we're even hitting that code here.
Comment 9 Peter van der Woude [:Peter6] 2006-04-12 09:55:07 PDT
(In reply to comment #8)
> No idea.  Ask in that bug? 

I can't , it's a security_group-only bug
Comment 10 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-12 18:45:50 PDT
I'm on this. It is indeed a regression from bug 321299. In particular, the problem is that the scope object introduced by that bug was assumed (and enforced) to be immutable once set, and document.open breaks that assumption. The fix is to make the scope object mutable, but this involves reparenting all of the wrappers in the old scope.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-12 22:21:43 PDT
Hmm... do we really want to make document.open give a new inner window?  I guess that's safer then clearing scope on the old one etc.... :(
Comment 12 Jesse Ruderman 2006-04-13 00:12:36 PDT
I'm curious why this only affects https Gmail, and I'm curious what a reduced testcase would look like.
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-13 16:33:36 PDT
Created attachment 218364 [details] [diff] [review]
Partial fix

This is not the final patch. I know of two problems with it (though I wouldn't be too surprised if there were more problems):
-- ReparentAllWrappersInScope is a misnomer for the new function, it should be named something like ReparentAllUniqueWrappersInScope.

-- I currently assert due to a "potential deadlock" while rewrapping wrappers. I'm still trying to figure out why.
Comment 14 Peter van der Woude [:Peter6] 2006-04-14 12:39:42 PDT
strange enough, Gmail works with a cairo build and http, but not with a non-cairo build and http
Comment 15 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-14 13:08:13 PDT
fwiw, I don't seem to be able to reproduce the assertion anymore.
Comment 16 Ria Klaassen (not reading all bugmail) 2006-04-17 00:01:36 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060416 Firefox/3.0a1
Still fails with the very last hourly.
Sometimes it works in a certain tab for some time, but when you open a new tab it fails again. When it fails it goes on failing. Right now I have one Gmail tab open that works (how long?) and four other dead ones.
You can only rely on 1.5x at the moment for Gmail.
Comment 17 Daniel Veditz [:dveditz] 2006-04-17 11:31:50 PDT
Plussing for 1.8.0.3 so we don't forget in case we take 321299
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 03:45:18 PDT
*** Bug 334475 has been marked as a duplicate of this bug. ***
Comment 19 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-18 21:54:58 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

I'm going to try to make a patch that avoids the nested locking. This is basically ready for review anyway, though.
Comment 20 Uri Bernstein (Google) 2006-04-19 07:20:00 PDT
This just happened to me with non-https gmail. So I guess it's a timing issue that is just much more likely to happen with https, for whatever reason.
Comment 21 Peter van der Woude [:Peter6] 2006-04-19 09:29:55 PDT
(In reply to comment #20)
> This just happened to me with non-https gmail. So I guess it's a timing issue
> that is just much more likely to happen with https, for whatever reason.
> 

Yeah, same here (it has been like that from the start), but maybe in just 5% of all cases while on https it's 100% of the time.
Comment 22 Ria Klaassen (not reading all bugmail) 2006-04-19 09:58:48 PDT
Sometimes I get slow script warnings.
Comment 23 Peter van der Woude [:Peter6] 2006-04-19 10:23:11 PDT
(In reply to comment #22)
> Sometimes I get slow script warnings.
> 
at least 50% of the time on https, almost never on http
Comment 24 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-20 15:42:36 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

Actually, avoiding the locking isn't as feasable as I'd hoped.
Comment 25 Tetsuji Rai 2006-04-21 02:51:32 PDT
*** Bug 334840 has been marked as a duplicate of this bug. ***
Comment 26 Daniel Veditz [:dveditz] 2006-04-24 10:59:17 PDT
How is this bug coming, is it feasible to fix for 1.8.0.3? Sounds like it's stalled.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2006-04-24 14:45:15 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

r+sr=jst
Comment 28 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-26 12:59:06 PDT
Fix checked into trunk.
Comment 29 Ria Klaassen (not reading all bugmail) 2006-04-26 13:11:09 PDT
Great! Very welcome patch.
Comment 30 Ben Turner (not reading bugmail, use the needinfo flag!) 2006-04-26 13:15:37 PDT
> Fix checked into trunk.

I believe this broke the trunk.(In reply to comment #28)
Comment 31 Peter van der Woude [:Peter6] 2006-04-26 14:57:23 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060426 Minefield/3.0a1 ID:2006042613 [cairo]

verified fixed, thanks
Comment 32 Tetsuji Rai 2006-04-27 05:12:35 PDT
(In reply to comment #28)
> Fix checked into trunk.
> 

Thanks!   Working like a charm!  And at the same time, gmail's design has changed...just a coincidence.
Comment 33 Stephen Donner [:stephend] 2006-04-27 19:59:36 PDT
Verified FIXED using 04-27-16 build of SeaMonkey trunk on Windows XP.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-28 02:01:25 PDT
Might be worth taking the patch to bug 335785 as well so this doesn't introduce a leak.
Comment 35 Daniel Veditz [:dveditz] 2006-05-02 11:26:28 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 36 Daniel Veditz [:dveditz] 2006-05-03 10:43:40 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

Unapproving for 1.8.0.4 and moving to next release per bug 321299 comment 36
Comment 37 Daniel Veditz [:dveditz] 2006-06-15 14:29:24 PDT
Comment on attachment 218364 [details] [diff] [review]
Partial fix

Jonas says we'll get a combined branch patch for all the related regressions.
Comment 38 Daniel Veditz [:dveditz] 2006-06-21 15:57:11 PDT
Moving to 1.8.0.6 to follow bug 321299
Comment 39 Daniel Veditz [:dveditz] 2006-07-06 14:20:35 PDT
If bug 321299 is blocking 1.8.1 then this should be also.
Comment 40 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-10 11:25:30 PDT
This is really fixed on the 1.8 branch already.
Comment 41 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-18 16:16:03 PDT
I checked a combined patch into the 1.8.0 branch.
Comment 42 Peter Van der Beken [:peterv] 2006-10-14 14:30:45 PDT
>+            JSObject *newParent = aOldScope;
>+            rv = sciWrapper.GetCallback()->PreCreate(identity, ccx, aOldScope,
>+                                                     &newParent);
...
>+            // Now, reparent the wrapper, since we know that it wants to be
>+            // reparented.
>+
>+            XPCWrappedNative *junk;
>+            rv = XPCWrappedNative::ReparentWrapperIfFound(ccx, oldScope,
>+                                                          newScope, aNewScope,
>+                                                          wrapper->GetIdentityObject(),
>+                                                          &junk);

Shouldn't this have used newParent as the parent instead of aNewScope?
Comment 43 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-10-16 10:51:49 PDT
Er... yes.  It should.  Bug 356851 filed.
Comment 44 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-10-16 11:31:56 PDT
(In reply to comment #42)=
> Shouldn't this have used newParent as the parent instead of aNewScope?

It doesn't matter. See the assertion inside the |if(newParent != aOldScope)| condition: the betterScope found from the newParent must be the same as aNewScope.
Comment 45 Peter Van der Beken [:peterv] 2006-10-16 11:44:48 PDT
Why would that matter? It just compares with the scope of newParent, that doesn't mean that newParent == aNewScope.
Comment 46 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-10-16 11:45:17 PDT
Sorry, I misread... Yeah, it should.

Note You need to log in before you can comment on or make changes to this bug.