If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

JS web progress listener is slow

RESOLVED FIXED

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: Tony Chang (Google), Assigned: Brett Wilson)

Tracking

({fixed1.8.1})

Trunk
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: on safebrowsing branch)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Safebrowsing uses a webprogresslistener to listen for when documents start loading.  It's currently implemented in javascript and receiving state change notifications (event to ignore them) is eating up a few ms.  A re-write in C++ with a JS callback should help get those ms back.
(Assignee)

Comment 1

12 years ago
Created attachment 222434 [details] [diff] [review]
Implementation in C++
Attachment #222434 - Flags: review?(tony)
(Reporter)

Comment 2

12 years ago
Comment on attachment 222434 [details] [diff] [review]
Implementation in C++

There's an extra printf in nsDocNavStartProgressListener.cpp (SetEnabled).

I don't think you need to check STATE_IS_REQEUEST or LOAD_DOCUMENT_URI because NOTIFY_STATE_DOCUMNET only fires in those cases.  Maybe they could become assertions.
Attachment #222434 - Flags: review?(tony) → review+
(Reporter)

Comment 3

12 years ago
(In reply to comment #1)
> Created an attachment (id=222434) [edit]
> Implementation in C++

Also, in browser/components/build/Makefile.in, the REQUIRES should probably be in the safebrowsing #ifdef.
(Assignee)

Comment 4

12 years ago
Created attachment 222509 [details] [diff] [review]
New patch

This removes the printf, and makes the check of the load flags an assertion. I kept the state flags check since we do seem to get calls for things other than STATE_IS_REQUEST.
Attachment #222434 - Attachment is obsolete: true
Attachment #222509 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Updated

12 years ago
Whiteboard: has patch

Comment 5

12 years ago
It looks like this patch creates a reference cycle here:

+  this.progressListener_.callback = this;

And, I don't see this cycle being broken anywhere.  Are you sure this doesn't leak?

Comment 6

12 years ago
fixed on SAFEBROWSING_20060516_BRANCH

if there is a follow-up patch to correct the (possible) leak, then i'll happily land that as well.  just let me know.
(Reporter)

Comment 7

12 years ago
Created attachment 222548 [details] [diff] [review]
fix crash and memory leak

Also fix the crash that's causing us to be on fire.
Attachment #222548 - Flags: review?(darin)

Updated

12 years ago
Attachment #222548 - Flags: review?(darin) → review+

Updated

12 years ago
Attachment #222509 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+

Comment 8

12 years ago
"fix crash and memory leak" fixed on SAFEBROWSING_20060516_BRANCH
(Assignee)

Updated

12 years ago
Whiteboard: has patch → on safebrowsing branch
(Reporter)

Comment 9

12 years ago
fixed on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.