Closed Bug 878395 Opened 7 years ago Closed 7 years ago

System Message API: a follow-up for bug 877627 to prevent wrong pages from handling the system messages


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed, b2g-v1.1hd fixed)

1.0.1 IOT3 (3jun)
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed
b2g-v1.1hd --- fixed


(Reporter: airpingu, Assigned: airpingu, NeedInfo)



(Whiteboard: [fixed-in-birch])


(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #877627 +++

Although Bug 877627 has been solved, I'd think it's not the best solution. I'm hoping to upload a follow-up patch with tef+ (please let me know if you don't think so).

We have to call .sendAsyncMessage(...) corresponding to a target (process). However, multiple windows can share the same target (process), so the content window needs to check if the manifest/page URL is matched for a fired system message. Only *one* window should handle that system message.

The current logic is fortunately working is because the

  - "SystemMessageManager:Message:Return:OK"
  - "SystemMessageManager:HandleMessagesDone"

are indexed and handled by the ["type", "manifest", "uri"] tuple, so even if the mismatched pages want to return these messages, the "uri" must not be matched in the parent. What I'm worry about is the handler, what's happening if two windows are both capable handling the same type? Wrong windows could redundantly handle the system message originally belonging to the correct one.
Attached patch Patch (obsolete) — Splinter Review
Please see comment #0. I believe we still need the page URL check at the content process, since multiple windows can share the same target (process). We still need the page URL check in the parent process because it can filter out the wrong targets in advance.

The solution at bug 877627 is still not complete. My bad. Fortunately, somehow it's still safe (please see comment #0).
Attachment #756932 - Flags: review?(fabrice)
Hi Fabrice, if you agree with patch, could you please go ahead to land this to

  - b2g18-v1.0.1
  - b2g18
  - inbound/birch
  - b2g-v1.1hd

when you're done reviewing, so that we can save some time.
Target Milestone: --- → 1.0.1 IOT3 (3jun)
Note that this fix won't be worse than the point in time we removed it. Let's be brave to land this. Otherwise, the page-URL-mismatched windows (within the same process) would keep trying to handle the wrong system message. It's dangerous.
Attached patch Patch, V1.1Splinter Review
Attachment #756932 - Attachment is obsolete: true
Attachment #756932 - Flags: review?(fabrice)
Attachment #756935 - Flags: review?(fabrice)
Attachment #756935 - Flags: review?(fabrice) → review+
Thanks Fabrice! Really appreciate your prompt help and working overtime!
Hi Gene, how about the fix on mozilla central? thanks
Flags: needinfo?(gene.lian)
(In reply to Joe Cheng [:jcheng] from comment #8)
> Hi Gene, how about the fix on mozilla central? thanks

This will land on m-c when birch will be merged.
As Fabrice said at comment #9, this patch is under way to land on the central.

Btw, I'd suggest this patch should also be applied as a follow-up of bug 877627, no matter for a new PVT build or for our vendor's testing build. It'd be better to consider these two bugs as a set.
Flags: needinfo?(gene.lian)
Hi Tang Juan,

Please see comment #10. I'd highly suggest including this patch for your build and testings, which is a follow-up for bug 877627.
Flags: needinfo?(tang.juan)
Daniel told me it's too late for now. Well, it doesn't really matter. If we still have chance then let's do it. If not, please remember to uplift this in the next round.

This patch is aimed for solving potential issues that didn't really happen in your/our previous tests (since your tests only show bug 877627 is the only bug).

That would be perfect if you could uplift this also. If you couldn't, then please don't worry about that. Everything should still be functioning.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.