Closed
Bug 1074983
Opened 10 years ago
Closed 4 years ago
Fix potential races with CTP messages between browser-plugins and PluginContent
Categories
(Firefox :: General, defect, P3)
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: mconley, Assigned: mconley)
References
Details
Bug 899347 made click-to-play work for remote browsers by using message passing between the content process and the parent process.
This, however, has opened us up to a few small race conditions. We've patched a few of the race conditions, but gfritzsche and I would like to find a more robust solution rather than plugging the races one by one.
The problem is like this:
Suppose I am on page A with some plugins. I travel to page B with some plugins, and then quickly browse back to page A.
The message for the page B content can sometimes appear after we've returned to page A. We've fixed some of these cases by doing Principal comparison between the page that we're handling PluginContent messages from, and the currently selected browser (this was done in bug 1070053). We also deal with the fact that Data URIs inherit the Principals of whoever linked to them by analyzing the locations of the page that send the PluginContent message and the location of the currently selected browser (this was done in bug 1069075).
This, however, does not account for the possibility of reloading the same page, and receiving messages from the content from before the reload _after_ the reload.
It seems like there should be a way of eliminating this entire class of races. We should find that way.
Comment 1•10 years ago
|
||
From my understanding, we should have a way to invalidate in-flight messages or discard them later if they are from before navigating.
I wonder if we don't have anything already that could let us identify that state.
Assignee | ||
Updated•10 years ago
|
tracking-e10s:
--- → ?
Updated•10 years ago
|
Assignee: nobody → mconley
Comment 2•10 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> From my understanding, we should have a way to invalidate in-flight messages
> or discard them later if they are from before navigating.
> I wonder if we don't have anything already that could let us identify that
> state.
Do we share inner window IDs between the content and main processes?
Assignee | ||
Comment 3•10 years ago
|
||
I don't think so - but that's a decent idea. We could send those up instead of locations. I'm not even sure we need the Principals anymore with that plan. What do you think, gfritzsche?
Flags: needinfo?(georg.fritzsche)
Assignee | ||
Comment 4•10 years ago
|
||
Although, hm - right - unsure if the parent ever gets those inner window IDs to compare against.
Let me think about this some more.
Flags: needinfo?(georg.fritzsche)
Assignee | ||
Comment 5•10 years ago
|
||
Just recording some conversation that billm and I had in IRC:
Part of the problem is that the dismissel / removal of notifications happens in the parent when the parent notices that we change locations. What might be better is for PopupNotifications.jsm and the notificationbox XBL binding to inject frame scripts into the target browsers, and use messages to notice when locations change. That'd solve all of the above problems for CTP, because the messages are guaranteed to be received in order - even if we've browsed away and we get messages for the last page we were on, we're also going to get a message to remove those notifications right after.
That might make testing CTP stuff a little tricky though - we'll need to wait for the PopupNotifications to be removed after browsing to a new page before proceeding.
Comment 6•10 years ago
|
||
Per last weeks talk, the most straight-forward (and most likely generally useful) way is probably to:
* have a counter in the content
* on every (real) navigation, increment it and message it to the parent
* add the current counter state to (all or some) messages
* now, thanks to defined message ordering, the parent code can compare its counter vs. the message counter
* if the messages counter is less than the parents counter, we can discard the message in certain scenarios (like the CTP issues here)
Mike, do you know who from e10s can confirm this is sane and not redundant to some other code we already have?
Flags: needinfo?(mconley)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> Per last weeks talk, the most straight-forward (and most likely generally
> useful) way is probably to:
> * have a counter in the content
> * on every (real) navigation, increment it and message it to the parent
> * add the current counter state to (all or some) messages
> * now, thanks to defined message ordering, the parent code can compare its
> counter vs. the message counter
> * if the messages counter is less than the parents counter, we can discard
> the message in certain scenarios (like the CTP issues here)
>
> Mike, do you know who from e10s can confirm this is sane and not redundant
> to some other code we already have?
I guess that's me. :) It's not redundant - SessionStore uses a somewhat similar scheme, but it's not something general that we can just tag on. I don't think it'll be too bad to handroll this - I'll put something up in a few days.
Flags: needinfo?(mconley)
Assignee | ||
Comment 9•9 years ago
|
||
Yes, I believe this is still a problem.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> I
> don't think it'll be too bad to handroll this - I'll put something up in a
> few days.
*sigh*
Flags: needinfo?(mconley)
Updated•9 years ago
|
Priority: -- → P3
Comment 10•4 years ago
|
||
We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•