Opened 5 years ago

Closed 5 years ago

#1724 closed change (fixed)

Messaging on Safari is prone to memory leaks

Reported by: trev Assignee: sebastian
Priority: P2 Milestone: Adblock-Plus-1.8.10-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: Blocked By:
Blocking: Platform: Safari
Ready: yes Confidential: no
Tester: Verified working:
Review URL(s):

http://codereview.adblockplus.org/6751260996796416

Description

Background

Our abstraction layer implements a way to respond to messages. While on Chrome this is a platform feature, on Safari we had to implement it ourselves. And it has the same issue that the Chrome implementation originally had: if the sender expects an answer but the receiver of the message doesn't send one, then the sender has no way of knowing it. So the callback for the answer will stay around waiting forever, hence creating a memory leak.

This is mostly a theoretical issue as to my knowledge all messages sent are being handled and always get a response.

What to change

Implement the same solution as in Chrome: if the message receiver wants to use the callback asynchronously then it needs to explicitly say so (return true). If it doesn't and it doesn't send out a response synchronously either, then the abstraction layer can send a "response" itself: "no response coming, don't wait any more".

Change History (6)

comment:1 Changed 5 years ago by sebastian

  • Cc sebastian added
  • Priority changed from Unknown to P2

Since #1708 we don't send an empty object as default (if there isn't a matching message type) anymore, increasing the potential of a memory leak here. So I think we should fix this rather sooner than later.

comment:2 Changed 5 years ago by sebastian

  • Cc sebastian removed
  • Owner set to sebastian

comment:3 Changed 5 years ago by sebastian

  • Resolution set to fixed
  • Review URL(s) modified (diff)
  • Status changed from new to closed

comment:4 Changed 5 years ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:5 Changed 5 years ago by sebastian

  • Status changed from reopened to reviewing

comment:6 Changed 5 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.