Closed
Bug 341364
Opened 19 years ago
Closed 19 years ago
Regression: "Are you sure you want to navigate away from this page?" dialog at Gmail
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
VERIFIED
FIXED
People
(Reporter: ria.klaassen, Assigned: mrbkap)
References
Details
(Keywords: regression, testcase)
Attachments
(2 files, 1 obsolete file)
|
13.69 KB,
image/png
|
Details | |
|
2.11 KB,
patch
|
bzbarsky
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
Screenshot of the problem
Since a few hours I see this message pop up everytime when I want to leave Gmail, even when it is only temporary.
Steps to reproduce:
- Load Gmail
- Load a bookmark over it or close window or tab
- Alert pops up
Regression between 1.9a1_2006061216 and 1.9a1_2006061220.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-12+15%3A00&maxdate=2006-06-12+21%3A00
Maybe I blame the wrong bug, but bug 255942 seems the most likely to me.
|
Reporter | |
Comment 1•19 years ago
|
||
|
||
Updated•19 years ago
|
Keywords: regression
Summary: Are you sure you want to navigate away from this page? :/ → Regression: "Are you sure you want to navigate away from this page?" dialog at Gmail
|
||
Comment 3•19 years ago
|
||
I've created two test builds from today, one without bug 255942 and one without both bug 255942 and bug 176182. Could you test these and see if either of them works?
http://gavinsharp.com/ff/firefox-3.0a1-2006-06-13-07-no255942.zip
http://gavinsharp.com/ff/firefox-3.0a1-2006-06-13-07-no255942-no176182.zip
|
|
Reporter | |
Comment 4•19 years ago
|
||
Thank you Gavin. No problem (no alert) with the first build.
So the dependency to bug 255942 can be added back again.
Blocks: dom-agnostic
|
||
Comment 5•19 years ago
|
||
Not just gmail: http://weblogs.mozillazine.org/asa/ is showing it for me, too, along with a few other sites.
|
|
||
Comment 6•19 years ago
|
||
On asa's blog it depends on the basicstats.js file being loaded. Further testcasing coming.
|
|
||
Comment 7•19 years ago
|
||
data:text/html,<script>window.onbeforeunload = function(){};</script>
Minimal testcase.
|
||
Comment 9•19 years ago
|
||
So the issue here is that nsJSEventListener::HandleEvent calls PreventDefault on any onbeforeunload event whose handler returns something not void. Or at least that's how it _used_ to work. Now we're checking for nsIDataType::VTYPE_VOID, but stuffing JSVAL_VOID into an XPCVariant makes it VTYPE_EMPTY (see XPCVariant::InitializeData). So we alert on any page that has an onbeforeunload handler.
Now we could check for VTYPE_EMPTY, but then we'd treat JSVAL_NULL the same way as JSVAL_VOID. Is that desirable? What does IE do on something like:
data:text/html,<body onbeforeunload="return null;">
? I've tested, and Opera and KHTML don't seem to support onbeforeunload, so IE compat is the only issue here.
Component: JavaScript Engine → DOM: Events
Keywords: testcase
In IE
<body onbeforeunload="return null;">
will bring up a dialog on exit asking if you are sure you want to leave with the message 'null'.
<body onbeforeunload="return;">
Will not pop up a dialog.
|
|
||
Comment 11•19 years ago
|
||
OK. So if we're going to keep IE compat here we need a way to differentiate between JSVAL_VOID and JSVAL_NULL in the variant we're using here. So either change XPCVariant, or use a different variant impl, or something.
Assignee: general → events
OS: Windows XP → All
QA Contact: general → ian
Hardware: PC → All
|
Assignee | |
Comment 12•19 years ago
|
||
This patch makes us distinguish between null and void in xpcvariant.
Assignee: events → mrbkap
Status: NEW → ASSIGNED
Attachment #225533 -
Flags: superreview?(shaver)
Attachment #225533 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Comment 13•19 years ago
|
||
Attachment #225533 -
Attachment is obsolete: true
Attachment #225535 -
Flags: superreview?(shaver)
Attachment #225535 -
Flags: review?(bzbarsky)
Attachment #225533 -
Flags: superreview?(shaver)
Attachment #225533 -
Flags: review?(bzbarsky)
|
|
||
Comment 14•19 years ago
|
||
Comment on attachment 225535 [details] [diff] [review]
Right patch, though
Assuming you looked at all current consumers of TYPE_EMPTY and TYPE_VOID (do we have any?) and they're chill with this, r=bzbarsky
Attachment #225535 -
Flags: review?(bzbarsky) → review+
|
||
Comment 15•19 years ago
|
||
FWIW, my lxr search shows that all consumers treat VTYPE_VOID and VTYPE_EMPTY the same. I believe an alternative change would be to have nsJSEnvironment::CallEventHandler check JSVAL_IS_VOID before calling XPConnect()->JSToVariant, but I think changing xpconnect to make the distinction is better.
|
||
Comment 16•19 years ago
|
||
Comment on attachment 225535 [details] [diff] [review]
Right patch, though
sr=shaver
Attachment #225535 -
Flags: superreview?(shaver) → superreview+
|
|
Assignee | |
Comment 17•19 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•