Greasemonkey 0.6.2 hangs on some pages since Firefox 1.4.1

VERIFIED FIXED in mozilla1.8beta5

Status

()

P1
critical
VERIFIED FIXED
14 years ago
14 years ago

People

(Reporter: Mardak, Assigned: mrbkap)

Tracking

({crash, regression, verified1.8})

1.8 Branch
mozilla1.8beta5
crash, regression, verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

14 years ago
Loading gmail with a user script activated cause the browser to freeze. It's not
specific to a particular user script because a blank one also causes the
problem, so something with gmail and injecting scripts. (But it's not limited to
just gmail.)

Perhaps related to bug 310742? Aaron would probably be best to know what in
greasemonkey might be causing this issue.

Pretty sure it's caused by the fix for 308856 because backing out the changes
causes this issue to go away.

Last working build is Firefox 1.4 20050930.
Depends on: 310742

Updated

14 years ago
Severity: normal → critical
(Reporter)

Comment 1

14 years ago
Just as an update..

Applying the patch for bug 310742 (attachment 198385 [details] [diff] [review]) does not solve this issue.
I had to back out the changes to jsapi.c/h and nsDOMClassInfo.cpp/h from bug 308856.
(Reporter)

Comment 2

14 years ago
CCing brendan: "Taking from mrbkap so he can focus on WinXP drop shadow fixage."
Flags: blocking1.8b5?
Who says this has anything to do with that other bug?  The only way to know is
to get a stack, or sample of stacks if the process accumulates CPU time, and
post them here.  Can someone please do that?

Another way (not for sure) is to binary search among past nightlies and even
finer grained by date builds, and point to the regression window and its
bonsai-reported checkin traffic.

/be
(Reporter)

Comment 4

14 years ago
Sorry if I wasn't clear with the bug description, but this started happening
with Firefox 1.4.1 meaning 20051001 builds. This is definately related to bug
308856 because backing out those patches fixes this issue. I only guessed it was
possibly related to bug 310742 because that also regressed from bug 308856, but
I was hoping Aaron could clarify as he probably understands the greasemonkey
code the best.

Switching depends to 308856.. fixing 310742 didn't help.
Depends on: 308856
No longer depends on: 310742
Sorry, somehow misread your comment 0 about backing out the entire patch for bug
308856 curing this bug's symptom.  Looks like I've got you on IRC, maybe we can
gdb to get a stack....

/be
(Reporter)

Comment 6

14 years ago
Posted file stack1
Stack trace from tiger's "report bug to apple"
(Reporter)

Comment 7

14 years ago
Posted file stack2
Ok, thanks to some stacks from Edward, this is clearly a stack overflow.  The JS
engine should detect it and not crash.

Not clear from the crash reports (in which the deep stack was cropped) what
script is causing this stack overflow with a rogue toSource call.  We really
should find out, if possible.  Look for a core file and run gdb on the app and
that core file.

/be
Assignee: general → brendan
Component: DOM → JavaScript Engine
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta5
(I mean its indirect callers, array_join_sub and js_obj_toSource -- because it
recursively walks the graph being toSource'd in addition to those callers,
which call themselves recursively to walk after marking on outermost call.)

This is as safe a patch as you can get, given our stack limiting.

There must be something GreaseMonkey-ish doing uneval() or obj.toSource() --
Aaron can you check?  It's risky on deep graphs, but it shouldn't crash you as
noted. It just might throw, so wrapping in try-catch helps, but why do it at
all if you can avoid it?  It's costly on large object graphs.

/be
Attachment #198412 - Flags: review?(mrbkap)
Attachment #198412 - Flags: approval1.8b5?
Comment on attachment 198412 [details] [diff] [review]
Have to check stack depth in MarkSharpObjects as well as its callers

r=mrbkap
Attachment #198412 - Flags: review?(mrbkap) → review+
Fixed on trunk.  Plus'ing, this is trivially safe and helps an unchecked
recursion path that can generate an overdeep stack.

/be
Status: NEW → ASSIGNED
Flags: blocking1.8b5? → blocking1.8b5+

Updated

14 years ago
Attachment #198412 - Flags: approval1.8b5? → approval1.8b5+
Fixed everywhere.  Thanks for reporting this and helping on IRC.  I changed the
hang keyword to crash (if your OS makes a crash look like a hang, it has a
"hang" bug ;-).

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: hang → crash, fixed1.8
Resolution: --- → FIXED
(Reporter)

Comment 13

14 years ago
Just for comparison after applying the patch.. 

Loading mail.google.com (logged in) takes ~4 seconds with greasemonkey no
scripts installed.
After installing a blank user script (runs * but does nothing), mail.google.com
loads in ~100 seconds.

Which is definately better than never without the patch. :) [On a side note, it
would take almost a minute before tiger killed firefox. I assumed it was hanging
when I first ran into the issue, so I manually killed it before the autokill/crash.]
(Reporter)

Comment 14

14 years ago
Posted file gm.logChrome gmail
Aaron, here's a log with greasemonkey.logChrome set to true for loading gmail
with a blank user script after applying the patch. The last function called
before the loonnngggg sequence of "<<<Done enumerating" is..

GM_GreasemonkeyService.injectScripts([object Object],
http://mail.google.com/mail/?view=page&name=js&ver=237a5136d7692b04, [object
Window])

mrbkap pointed to the utils.js file line 391 which later uses args.join (But
I'm not sure why it would cause this issue..)
Depends on: 311022
This is not GreaseMonkey's fault.

There's a separate bug here, in the split window code.  We are marking the
window.arguments object, whose '0' property is a window, whose 'window' property
is the same window, but it keeps getting rewrapped by a new XPCWrappedNative, ad
infinitum.

I've filed bug 311022 on this.  It should block 1.8 / Firefox 1.5.

/be
No longer depends on: 311022
Reopening so this symptom report can catch dups.  Should mark FIXED when bug
311022 is fixed.

/be
Status: RESOLVED → REOPENED
Depends on: 311022
Resolution: FIXED → ---
mrbkap is all over this.  He just said on IRC that it has to do with automatic
implicit deep XPCNativeWrappers and window.window, window.window.window, etc.

Aaron, can you jump on IRC today?

/be
Assignee: brendan → mrbkap
Status: REOPENED → NEW
(Reporter)

Comment 18

14 years ago
CCing Jeremy as he has worked quite a bit on greasemonkey as well. (And seems to
be more available recently looking at the greasemonkey mailing list.)

Could you help us out and stop by IRC?

Comment 19

14 years ago
I'll be able to get on IRC in a 2-3 hours.  Unfortunately, my corp blocks IRC
ports and I haven't gotten around to tunneling through it.

Thanks for CC'ing me, Edward.

Aaron's on very limited connectivity for the next week and a half.
Marking this fixed since bug 311022 is now fixed.
Status: NEW → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

14 years ago
Things are working like before now. :)
Status: RESOLVED → VERIFIED

Updated

14 years ago
Flags: testcase-

Comment 22

14 years ago
verifying per Edward's comment
Keywords: fixed1.8 → verified1.8
You need to log in before you can comment on or make changes to this bug.